[PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[Louis Li]

[Why]
External monitor cannot be displayed consistently, if connecting
via this Apple dongle (A1621, USB Type-C to HDMI).
By experiments, it is confirmed that the dongle needs 200ms at least
to be ready for communication, after it sets HPD signal high.

[How]
When receiving HPD IRQ, delay 300ms at the beginning of handle_hpd_irq().
Then run the original procedure.
With this patch applied, the problem cannot be reproduced.
With other dongles, test results are PASS.
Test result is PASS to plug in HDMI cable while playing video,
and the video is being playing smoothly.
Test result is PASS after system resumes from suspend.

Signed-off-by: Louis Li <Ching-shih.Li@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
 1 file changed, 5 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 0aef92b7c037..5b844b6a5a59 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1025,6 +1025,11 @@ static void handle_hpd_irq(void *param)
 	struct drm_device *dev = connector->dev;
 	enum dc_connection_type new_connection_type = dc_connection_none;
 
+	/* Some monitors/dongles need around 200ms to be ready for communication
+	 * after those devices drive HPD signal high.
+	 */
+	msleep(300);
+
 	/* In case of failure or MST no need to update connector status or notify the OS
 	 * since (for MST case) MST does this in it's own context.
 	 */
-- 
2.21.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[Louis Li]

[Why]
External monitor cannot be displayed consistently, if connecting
via this Apple dongle (A1621, USB Type-C to HDMI).
By experiments, it is confirmed that the dongle needs 200ms at least
to be ready for communication, after it sets HPD signal high.

[How]
When receiving HPD IRQ, delay 300ms at the beginning of handle_hpd_irq().
Then run the original procedure.
With this patch applied, the problem cannot be reproduced.
With other dongles, test results are PASS.
Test result is PASS to plug in HDMI cable while playing video,
and the video is being playing smoothly.
Test result is PASS after system resumes from suspend.

Signed-off-by: Louis Li <Ching-shih.Li@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
 1 file changed, 5 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 0aef92b7c037..5b844b6a5a59 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1025,6 +1025,11 @@ static void handle_hpd_irq(void *param)
 	struct drm_device *dev = connector->dev;
 	enum dc_connection_type new_connection_type = dc_connection_none;
 
+	/* Some monitors/dongles need around 200ms to be ready for communication
+	 * after those devices drive HPD signal high.
+	 */
+	msleep(300);
+
 	/* In case of failure or MST no need to update connector status or notify the OS
 	 * since (for MST case) MST does this in it's own context.
 	 */
-- 
2.21.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[Li, Ching-shih (Louis)]

Hi reviewers,

What is the review result for this patch? Customer is pushing on this change to merge. TKS for your attention.

BR,
Louis

-----Original Message-----
From: Louis Li <Ching-shih.Li@amd.com> 
Sent: Thursday, November 14, 2019 11:42 AM
To: amd-gfx@lists.freedesktop.org
Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; Li, Ching-shih (Louis) <Ching-shih.Li@amd.com>
Subject: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[Why]
External monitor cannot be displayed consistently, if connecting via this Apple dongle (A1621, USB Type-C to HDMI).
By experiments, it is confirmed that the dongle needs 200ms at least to be ready for communication, after it sets HPD signal high.

[How]
When receiving HPD IRQ, delay 300ms at the beginning of handle_hpd_irq().
Then run the original procedure.
With this patch applied, the problem cannot be reproduced.
With other dongles, test results are PASS.
Test result is PASS to plug in HDMI cable while playing video, and the video is being playing smoothly.
Test result is PASS after system resumes from suspend.

Signed-off-by: Louis Li <Ching-shih.Li@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
 1 file changed, 5 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 0aef92b7c037..5b844b6a5a59 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1025,6 +1025,11 @@ static void handle_hpd_irq(void *param)
 	struct drm_device *dev = connector->dev;
 	enum dc_connection_type new_connection_type = dc_connection_none;
 
+	/* Some monitors/dongles need around 200ms to be ready for communication
+	 * after those devices drive HPD signal high.
+	 */
+	msleep(300);
+
 	/* In case of failure or MST no need to update connector status or notify the OS
 	 * since (for MST case) MST does this in it's own context.
 	 */
--
2.21.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[Li, Ching-shih (Louis)]

Hi reviewers,

What is the review result for this patch? Customer is pushing on this change to merge. TKS for your attention.

BR,
Louis

-----Original Message-----
From: Louis Li <Ching-shih.Li@amd.com> 
Sent: Thursday, November 14, 2019 11:42 AM
To: amd-gfx@lists.freedesktop.org
Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; Li, Ching-shih (Louis) <Ching-shih.Li@amd.com>
Subject: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[Why]
External monitor cannot be displayed consistently, if connecting via this Apple dongle (A1621, USB Type-C to HDMI).
By experiments, it is confirmed that the dongle needs 200ms at least to be ready for communication, after it sets HPD signal high.

[How]
When receiving HPD IRQ, delay 300ms at the beginning of handle_hpd_irq().
Then run the original procedure.
With this patch applied, the problem cannot be reproduced.
With other dongles, test results are PASS.
Test result is PASS to plug in HDMI cable while playing video, and the video is being playing smoothly.
Test result is PASS after system resumes from suspend.

Signed-off-by: Louis Li <Ching-shih.Li@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
 1 file changed, 5 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 0aef92b7c037..5b844b6a5a59 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1025,6 +1025,11 @@ static void handle_hpd_irq(void *param)
 	struct drm_device *dev = connector->dev;
 	enum dc_connection_type new_connection_type = dc_connection_none;
 
+	/* Some monitors/dongles need around 200ms to be ready for communication
+	 * after those devices drive HPD signal high.
+	 */
+	msleep(300);
+
 	/* In case of failure or MST no need to update connector status or notify the OS
 	 * since (for MST case) MST does this in it's own context.
 	 */
--
2.21.0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[Kazlauskas, Nicholas]

On 2019-11-21 3:31 a.m., Li, Ching-shih (Louis) wrote:
> Hi reviewers,
> 
> What is the review result for this patch? Customer is pushing on this change to merge. TKS for your attention.
> 
> BR,
> Louis
> 
> -----Original Message-----
> From: Louis Li <Ching-shih.Li@amd.com>
> Sent: Thursday, November 14, 2019 11:42 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; Li, Ching-shih (Louis) <Ching-shih.Li@amd.com>
> Subject: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected
> 
> [Why]
> External monitor cannot be displayed consistently, if connecting via this Apple dongle (A1621, USB Type-C to HDMI).
> By experiments, it is confirmed that the dongle needs 200ms at least to be ready for communication, after it sets HPD signal high.
> 
> [How]
> When receiving HPD IRQ, delay 300ms at the beginning of handle_hpd_irq().
> Then run the original procedure.
> With this patch applied, the problem cannot be reproduced.
> With other dongles, test results are PASS.
> Test result is PASS to plug in HDMI cable while playing video, and the video is being playing smoothly.
> Test result is PASS after system resumes from suspend.
> 
> Signed-off-by: Louis Li <Ching-shih.Li@amd.com>

This is still a NAK from me since the logic hasn't changed from the 
first patch.

Sleeps don't belong in IRQ handlers.

Regards,
Nicholas Kazlauskas

> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
>   1 file changed, 5 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 0aef92b7c037..5b844b6a5a59 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1025,6 +1025,11 @@ static void handle_hpd_irq(void *param)
>   	struct drm_device *dev = connector->dev;
>   	enum dc_connection_type new_connection_type = dc_connection_none;
>   
> +	/* Some monitors/dongles need around 200ms to be ready for communication
> +	 * after those devices drive HPD signal high.
> +	 */
> +	msleep(300);
> +
>   	/* In case of failure or MST no need to update connector status or notify the OS
>   	 * since (for MST case) MST does this in it's own context.
>   	 */
> --
> 2.21.0
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[Kazlauskas, Nicholas]

On 2019-11-21 3:31 a.m., Li, Ching-shih (Louis) wrote:
> Hi reviewers,
> 
> What is the review result for this patch? Customer is pushing on this change to merge. TKS for your attention.
> 
> BR,
> Louis
> 
> -----Original Message-----
> From: Louis Li <Ching-shih.Li@amd.com>
> Sent: Thursday, November 14, 2019 11:42 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; Li, Ching-shih (Louis) <Ching-shih.Li@amd.com>
> Subject: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected
> 
> [Why]
> External monitor cannot be displayed consistently, if connecting via this Apple dongle (A1621, USB Type-C to HDMI).
> By experiments, it is confirmed that the dongle needs 200ms at least to be ready for communication, after it sets HPD signal high.
> 
> [How]
> When receiving HPD IRQ, delay 300ms at the beginning of handle_hpd_irq().
> Then run the original procedure.
> With this patch applied, the problem cannot be reproduced.
> With other dongles, test results are PASS.
> Test result is PASS to plug in HDMI cable while playing video, and the video is being playing smoothly.
> Test result is PASS after system resumes from suspend.
> 
> Signed-off-by: Louis Li <Ching-shih.Li@amd.com>

This is still a NAK from me since the logic hasn't changed from the 
first patch.

Sleeps don't belong in IRQ handlers.

Regards,
Nicholas Kazlauskas

> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
>   1 file changed, 5 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 0aef92b7c037..5b844b6a5a59 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1025,6 +1025,11 @@ static void handle_hpd_irq(void *param)
>   	struct drm_device *dev = connector->dev;
>   	enum dc_connection_type new_connection_type = dc_connection_none;
>   
> +	/* Some monitors/dongles need around 200ms to be ready for communication
> +	 * after those devices drive HPD signal high.
> +	 */
> +	msleep(300);
> +
>   	/* In case of failure or MST no need to update connector status or notify the OS
>   	 * since (for MST case) MST does this in it's own context.
>   	 */
> --
> 2.21.0
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[Kazlauskas, Nicholas]

On 2019-11-21 8:40 a.m., Kazlauskas, Nicholas wrote:
> On 2019-11-21 3:31 a.m., Li, Ching-shih (Louis) wrote:
>> Hi reviewers,
>>
>> What is the review result for this patch? Customer is pushing on this 
>> change to merge. TKS for your attention.
>>
>> BR,
>> Louis
>>
>> -----Original Message-----
>> From: Louis Li <Ching-shih.Li@amd.com>
>> Sent: Thursday, November 14, 2019 11:42 AM
>> To: amd-gfx@lists.freedesktop.org
>> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, 
>> Harry <Harry.Wentland@amd.com>; Li, Ching-shih (Louis) 
>> <Ching-shih.Li@amd.com>
>> Subject: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be 
>> successfully detected
>>
>> [Why]
>> External monitor cannot be displayed consistently, if connecting via 
>> this Apple dongle (A1621, USB Type-C to HDMI).
>> By experiments, it is confirmed that the dongle needs 200ms at least 
>> to be ready for communication, after it sets HPD signal high.
>>
>> [How]
>> When receiving HPD IRQ, delay 300ms at the beginning of handle_hpd_irq().
>> Then run the original procedure.
>> With this patch applied, the problem cannot be reproduced.
>> With other dongles, test results are PASS.
>> Test result is PASS to plug in HDMI cable while playing video, and the 
>> video is being playing smoothly.
>> Test result is PASS after system resumes from suspend.
>>
>> Signed-off-by: Louis Li <Ching-shih.Li@amd.com>
> 
> This is still a NAK from me since the logic hasn't changed from the 
> first patch.
> 
> Sleeps don't belong in IRQ handlers.
> 
> Regards,
> Nicholas Kazlauskas

Actually, this lives in the low IRQ context which means that it's 
already been queued off to a work thread so it's safe to sleep.

I'm not sure we want a general 300ms sleep (even by experiment you said 
that it only needed 200ms) for all connectors.

Nicholas Kazlauskas

> 
>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
>>   1 file changed, 5 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 0aef92b7c037..5b844b6a5a59 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -1025,6 +1025,11 @@ static void handle_hpd_irq(void *param)
>>       struct drm_device *dev = connector->dev;
>>       enum dc_connection_type new_connection_type = dc_connection_none;
>> +    /* Some monitors/dongles need around 200ms to be ready for 
>> communication
>> +     * after those devices drive HPD signal high.
>> +     */
>> +    msleep(300);
>> +
>>       /* In case of failure or MST no need to update connector status 
>> or notify the OS
>>        * since (for MST case) MST does this in it's own context.
>>        */
>> -- 
>> 2.21.0
>>
> 
> _______________________________________________
> 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: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[Kazlauskas, Nicholas]

On 2019-11-21 8:40 a.m., Kazlauskas, Nicholas wrote:
> On 2019-11-21 3:31 a.m., Li, Ching-shih (Louis) wrote:
>> Hi reviewers,
>>
>> What is the review result for this patch? Customer is pushing on this 
>> change to merge. TKS for your attention.
>>
>> BR,
>> Louis
>>
>> -----Original Message-----
>> From: Louis Li <Ching-shih.Li@amd.com>
>> Sent: Thursday, November 14, 2019 11:42 AM
>> To: amd-gfx@lists.freedesktop.org
>> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, 
>> Harry <Harry.Wentland@amd.com>; Li, Ching-shih (Louis) 
>> <Ching-shih.Li@amd.com>
>> Subject: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be 
>> successfully detected
>>
>> [Why]
>> External monitor cannot be displayed consistently, if connecting via 
>> this Apple dongle (A1621, USB Type-C to HDMI).
>> By experiments, it is confirmed that the dongle needs 200ms at least 
>> to be ready for communication, after it sets HPD signal high.
>>
>> [How]
>> When receiving HPD IRQ, delay 300ms at the beginning of handle_hpd_irq().
>> Then run the original procedure.
>> With this patch applied, the problem cannot be reproduced.
>> With other dongles, test results are PASS.
>> Test result is PASS to plug in HDMI cable while playing video, and the 
>> video is being playing smoothly.
>> Test result is PASS after system resumes from suspend.
>>
>> Signed-off-by: Louis Li <Ching-shih.Li@amd.com>
> 
> This is still a NAK from me since the logic hasn't changed from the 
> first patch.
> 
> Sleeps don't belong in IRQ handlers.
> 
> Regards,
> Nicholas Kazlauskas

Actually, this lives in the low IRQ context which means that it's 
already been queued off to a work thread so it's safe to sleep.

I'm not sure we want a general 300ms sleep (even by experiment you said 
that it only needed 200ms) for all connectors.

Nicholas Kazlauskas

> 
>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
>>   1 file changed, 5 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 0aef92b7c037..5b844b6a5a59 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -1025,6 +1025,11 @@ static void handle_hpd_irq(void *param)
>>       struct drm_device *dev = connector->dev;
>>       enum dc_connection_type new_connection_type = dc_connection_none;
>> +    /* Some monitors/dongles need around 200ms to be ready for 
>> communication
>> +     * after those devices drive HPD signal high.
>> +     */
>> +    msleep(300);
>> +
>>       /* In case of failure or MST no need to update connector status 
>> or notify the OS
>>        * since (for MST case) MST does this in it's own context.
>>        */
>> -- 
>> 2.21.0
>>
> 
> _______________________________________________
> 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: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[Louis Li]

On Thu, Nov 21, 2019 at 08:47:50AM -0500, Kazlauskas, Nicholas wrote:
> On 2019-11-21 8:40 a.m., Kazlauskas, Nicholas wrote:
> >On 2019-11-21 3:31 a.m., Li, Ching-shih (Louis) wrote:
> >>Hi reviewers,
> >>
> >>What is the review result for this patch? Customer is pushing on this
> >>change to merge. TKS for your attention.
> >>
> >>BR,
> >>Louis
> >>
> >>-----Original Message-----
> >>From: Louis Li <Ching-shih.Li@amd.com>
> >>Sent: Thursday, November 14, 2019 11:42 AM
> >>To: amd-gfx@lists.freedesktop.org
> >>Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry
> >><Harry.Wentland@amd.com>; Li, Ching-shih (Louis) <Ching-shih.Li@amd.com>
> >>Subject: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be
> >>successfully detected
> >>
> >>[Why]
> >>External monitor cannot be displayed consistently, if connecting via
> >>this Apple dongle (A1621, USB Type-C to HDMI).
> >>By experiments, it is confirmed that the dongle needs 200ms at least to
> >>be ready for communication, after it sets HPD signal high.
> >>
> >>[How]
> >>When receiving HPD IRQ, delay 300ms at the beginning of handle_hpd_irq().
> >>Then run the original procedure.
> >>With this patch applied, the problem cannot be reproduced.
> >>With other dongles, test results are PASS.
> >>Test result is PASS to plug in HDMI cable while playing video, and the
> >>video is being playing smoothly.
> >>Test result is PASS after system resumes from suspend.
> >>
> >>Signed-off-by: Louis Li <Ching-shih.Li@amd.com>
> >
> >This is still a NAK from me since the logic hasn't changed from the first
> >patch.
> >
> >Sleeps don't belong in IRQ handlers.
> >
> >Regards,
> >Nicholas Kazlauskas
> 
> Actually, this lives in the low IRQ context which means that it's already
> been queued off to a work thread so it's safe to sleep.
> 
> I'm not sure we want a general 300ms sleep (even by experiment you said that
> it only needed 200ms) for all connectors.
> 
> Nicholas Kazlauskas
> 

Yes, it is IRQ context. Safe to call sleep(). Moreover, in current driver,
even udelay() is called in wait_for_training_aux_rd_interval() in the flow
of handle_hpd_irq().

For 2nd question, of course not all connectors have this behavior.
Based on real cases we ever dealt, some dongles like this, or some
monitors driven by TCON, have same behavior. And no chance to read
out anything to decide if delay is needed. This change does help
to have our driver gain better compatibility. Truly this should be
problem of dongles/monitors. We are not the only one to
workaround such a problem. This change does not hurt other connects,
and some other dongles are tested ok, e.g. HP/Huwai dongles, etc.

> >
> >>---
> >>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
> >>  1 file changed, 5 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 0aef92b7c037..5b844b6a5a59 100644
> >>--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>@@ -1025,6 +1025,11 @@ static void handle_hpd_irq(void *param)
> >>      struct drm_device *dev = connector->dev;
> >>      enum dc_connection_type new_connection_type = dc_connection_none;
> >>+    /* Some monitors/dongles need around 200ms to be ready for
> >>communication
> >>+     * after those devices drive HPD signal high.
> >>+     */
> >>+    msleep(300);
> >>+
> >>      /* In case of failure or MST no need to update connector status or
> >>notify the OS
> >>       * since (for MST case) MST does this in it's own context.
> >>       */
> >>-- 
> >>2.21.0
> >>
> >
> >_______________________________________________
> >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: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[Louis Li]

On Thu, Nov 21, 2019 at 08:47:50AM -0500, Kazlauskas, Nicholas wrote:
> On 2019-11-21 8:40 a.m., Kazlauskas, Nicholas wrote:
> >On 2019-11-21 3:31 a.m., Li, Ching-shih (Louis) wrote:
> >>Hi reviewers,
> >>
> >>What is the review result for this patch? Customer is pushing on this
> >>change to merge. TKS for your attention.
> >>
> >>BR,
> >>Louis
> >>
> >>-----Original Message-----
> >>From: Louis Li <Ching-shih.Li@amd.com>
> >>Sent: Thursday, November 14, 2019 11:42 AM
> >>To: amd-gfx@lists.freedesktop.org
> >>Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry
> >><Harry.Wentland@amd.com>; Li, Ching-shih (Louis) <Ching-shih.Li@amd.com>
> >>Subject: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be
> >>successfully detected
> >>
> >>[Why]
> >>External monitor cannot be displayed consistently, if connecting via
> >>this Apple dongle (A1621, USB Type-C to HDMI).
> >>By experiments, it is confirmed that the dongle needs 200ms at least to
> >>be ready for communication, after it sets HPD signal high.
> >>
> >>[How]
> >>When receiving HPD IRQ, delay 300ms at the beginning of handle_hpd_irq().
> >>Then run the original procedure.
> >>With this patch applied, the problem cannot be reproduced.
> >>With other dongles, test results are PASS.
> >>Test result is PASS to plug in HDMI cable while playing video, and the
> >>video is being playing smoothly.
> >>Test result is PASS after system resumes from suspend.
> >>
> >>Signed-off-by: Louis Li <Ching-shih.Li@amd.com>
> >
> >This is still a NAK from me since the logic hasn't changed from the first
> >patch.
> >
> >Sleeps don't belong in IRQ handlers.
> >
> >Regards,
> >Nicholas Kazlauskas
> 
> Actually, this lives in the low IRQ context which means that it's already
> been queued off to a work thread so it's safe to sleep.
> 
> I'm not sure we want a general 300ms sleep (even by experiment you said that
> it only needed 200ms) for all connectors.
> 
> Nicholas Kazlauskas
> 

Yes, it is IRQ context. Safe to call sleep(). Moreover, in current driver,
even udelay() is called in wait_for_training_aux_rd_interval() in the flow
of handle_hpd_irq().

For 2nd question, of course not all connectors have this behavior.
Based on real cases we ever dealt, some dongles like this, or some
monitors driven by TCON, have same behavior. And no chance to read
out anything to decide if delay is needed. This change does help
to have our driver gain better compatibility. Truly this should be
problem of dongles/monitors. We are not the only one to
workaround such a problem. This change does not hurt other connects,
and some other dongles are tested ok, e.g. HP/Huwai dongles, etc.

> >
> >>---
> >>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
> >>  1 file changed, 5 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 0aef92b7c037..5b844b6a5a59 100644
> >>--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>@@ -1025,6 +1025,11 @@ static void handle_hpd_irq(void *param)
> >>      struct drm_device *dev = connector->dev;
> >>      enum dc_connection_type new_connection_type = dc_connection_none;
> >>+    /* Some monitors/dongles need around 200ms to be ready for
> >>communication
> >>+     * after those devices drive HPD signal high.
> >>+     */
> >>+    msleep(300);
> >>+
> >>      /* In case of failure or MST no need to update connector status or
> >>notify the OS
> >>       * since (for MST case) MST does this in it's own context.
> >>       */
> >>-- 
> >>2.21.0
> >>
> >
> >_______________________________________________
> >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: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[Harry Wentland]

On 2019-11-22 1:33 a.m., Louis Li wrote:
> On Thu, Nov 21, 2019 at 08:47:50AM -0500, Kazlauskas, Nicholas wrote:
>> On 2019-11-21 8:40 a.m., Kazlauskas, Nicholas wrote:
>>> On 2019-11-21 3:31 a.m., Li, Ching-shih (Louis) wrote:
>>>> Hi reviewers,
>>>>
>>>> What is the review result for this patch? Customer is pushing on this
>>>> change to merge. TKS for your attention.
>>>>
>>>> BR,
>>>> Louis
>>>>
>>>> -----Original Message-----
>>>> From: Louis Li <Ching-shih.Li@amd.com>
>>>> Sent: Thursday, November 14, 2019 11:42 AM
>>>> To: amd-gfx@lists.freedesktop.org
>>>> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry
>>>> <Harry.Wentland@amd.com>; Li, Ching-shih (Louis) <Ching-shih.Li@amd.com>
>>>> Subject: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be
>>>> successfully detected
>>>>
>>>> [Why]
>>>> External monitor cannot be displayed consistently, if connecting via
>>>> this Apple dongle (A1621, USB Type-C to HDMI).
>>>> By experiments, it is confirmed that the dongle needs 200ms at least to
>>>> be ready for communication, after it sets HPD signal high.
>>>>
>>>> [How]
>>>> When receiving HPD IRQ, delay 300ms at the beginning of handle_hpd_irq().
>>>> Then run the original procedure.
>>>> With this patch applied, the problem cannot be reproduced.
>>>> With other dongles, test results are PASS.
>>>> Test result is PASS to plug in HDMI cable while playing video, and the
>>>> video is being playing smoothly.
>>>> Test result is PASS after system resumes from suspend.
>>>>
>>>> Signed-off-by: Louis Li <Ching-shih.Li@amd.com>
>>>
>>> This is still a NAK from me since the logic hasn't changed from the first
>>> patch.
>>>
>>> Sleeps don't belong in IRQ handlers.
>>>
>>> Regards,
>>> Nicholas Kazlauskas
>>
>> Actually, this lives in the low IRQ context which means that it's already
>> been queued off to a work thread so it's safe to sleep.
>>
>> I'm not sure we want a general 300ms sleep (even by experiment you said that
>> it only needed 200ms) for all connectors.
>>
>> Nicholas Kazlauskas
>>
> 
> Yes, it is IRQ context. Safe to call sleep(). Moreover, in current driver,
> even udelay() is called in wait_for_training_aux_rd_interval() in the flow
> of handle_hpd_irq().
> 
> For 2nd question, of course not all connectors have this behavior.
> Based on real cases we ever dealt, some dongles like this, or some
> monitors driven by TCON, have same behavior. And no chance to read
> out anything to decide if delay is needed. This change does help
> to have our driver gain better compatibility. Truly this should be
> problem of dongles/monitors. We are not the only one to
> workaround such a problem. This change does not hurt other connects,
> and some other dongles are tested ok, e.g. HP/Huwai dongles, etc.
> 

I still don't like this change. It might impact other use cases, such as
SST-to-MST switching on MST displays.

Have you checked how Windows deals with this dongle and how the Windows
team solved this? Have you checked how other drivers (such as i915) deal
with this dongle?

Have you checked whether you can pass DP compliance with this change?

Harry

>>>
>>>> ---
>>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
>>>>   1 file changed, 5 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 0aef92b7c037..5b844b6a5a59 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> @@ -1025,6 +1025,11 @@ static void handle_hpd_irq(void *param)
>>>>       struct drm_device *dev = connector->dev;
>>>>       enum dc_connection_type new_connection_type = dc_connection_none;
>>>> +    /* Some monitors/dongles need around 200ms to be ready for
>>>> communication
>>>> +     * after those devices drive HPD signal high.
>>>> +     */
>>>> +    msleep(300);
>>>> +
>>>>       /* In case of failure or MST no need to update connector status or
>>>> notify the OS
>>>>        * since (for MST case) MST does this in it's own context.
>>>>        */
>>>> -- 
>>>> 2.21.0
>>>>
>>>
>>> _______________________________________________
>>> 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: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[Harry Wentland]

On 2019-11-22 1:33 a.m., Louis Li wrote:
> On Thu, Nov 21, 2019 at 08:47:50AM -0500, Kazlauskas, Nicholas wrote:
>> On 2019-11-21 8:40 a.m., Kazlauskas, Nicholas wrote:
>>> On 2019-11-21 3:31 a.m., Li, Ching-shih (Louis) wrote:
>>>> Hi reviewers,
>>>>
>>>> What is the review result for this patch? Customer is pushing on this
>>>> change to merge. TKS for your attention.
>>>>
>>>> BR,
>>>> Louis
>>>>
>>>> -----Original Message-----
>>>> From: Louis Li <Ching-shih.Li@amd.com>
>>>> Sent: Thursday, November 14, 2019 11:42 AM
>>>> To: amd-gfx@lists.freedesktop.org
>>>> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry
>>>> <Harry.Wentland@amd.com>; Li, Ching-shih (Louis) <Ching-shih.Li@amd.com>
>>>> Subject: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be
>>>> successfully detected
>>>>
>>>> [Why]
>>>> External monitor cannot be displayed consistently, if connecting via
>>>> this Apple dongle (A1621, USB Type-C to HDMI).
>>>> By experiments, it is confirmed that the dongle needs 200ms at least to
>>>> be ready for communication, after it sets HPD signal high.
>>>>
>>>> [How]
>>>> When receiving HPD IRQ, delay 300ms at the beginning of handle_hpd_irq().
>>>> Then run the original procedure.
>>>> With this patch applied, the problem cannot be reproduced.
>>>> With other dongles, test results are PASS.
>>>> Test result is PASS to plug in HDMI cable while playing video, and the
>>>> video is being playing smoothly.
>>>> Test result is PASS after system resumes from suspend.
>>>>
>>>> Signed-off-by: Louis Li <Ching-shih.Li@amd.com>
>>>
>>> This is still a NAK from me since the logic hasn't changed from the first
>>> patch.
>>>
>>> Sleeps don't belong in IRQ handlers.
>>>
>>> Regards,
>>> Nicholas Kazlauskas
>>
>> Actually, this lives in the low IRQ context which means that it's already
>> been queued off to a work thread so it's safe to sleep.
>>
>> I'm not sure we want a general 300ms sleep (even by experiment you said that
>> it only needed 200ms) for all connectors.
>>
>> Nicholas Kazlauskas
>>
> 
> Yes, it is IRQ context. Safe to call sleep(). Moreover, in current driver,
> even udelay() is called in wait_for_training_aux_rd_interval() in the flow
> of handle_hpd_irq().
> 
> For 2nd question, of course not all connectors have this behavior.
> Based on real cases we ever dealt, some dongles like this, or some
> monitors driven by TCON, have same behavior. And no chance to read
> out anything to decide if delay is needed. This change does help
> to have our driver gain better compatibility. Truly this should be
> problem of dongles/monitors. We are not the only one to
> workaround such a problem. This change does not hurt other connects,
> and some other dongles are tested ok, e.g. HP/Huwai dongles, etc.
> 

I still don't like this change. It might impact other use cases, such as
SST-to-MST switching on MST displays.

Have you checked how Windows deals with this dongle and how the Windows
team solved this? Have you checked how other drivers (such as i915) deal
with this dongle?

Have you checked whether you can pass DP compliance with this change?

Harry

>>>
>>>> ---
>>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
>>>>   1 file changed, 5 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 0aef92b7c037..5b844b6a5a59 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> @@ -1025,6 +1025,11 @@ static void handle_hpd_irq(void *param)
>>>>       struct drm_device *dev = connector->dev;
>>>>       enum dc_connection_type new_connection_type = dc_connection_none;
>>>> +    /* Some monitors/dongles need around 200ms to be ready for
>>>> communication
>>>> +     * after those devices drive HPD signal high.
>>>> +     */
>>>> +    msleep(300);
>>>> +
>>>>       /* In case of failure or MST no need to update connector status or
>>>> notify the OS
>>>>        * since (for MST case) MST does this in it's own context.
>>>>        */
>>>> -- 
>>>> 2.21.0
>>>>
>>>
>>> _______________________________________________
>>> 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: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[Louis Li]

On Fri, Nov 22, 2019 at 10:31:19AM -0500, Harry Wentland wrote:
> 
> 
> On 2019-11-22 1:33 a.m., Louis Li wrote:
> > On Thu, Nov 21, 2019 at 08:47:50AM -0500, Kazlauskas, Nicholas wrote:
> >> On 2019-11-21 8:40 a.m., Kazlauskas, Nicholas wrote:
> >>> On 2019-11-21 3:31 a.m., Li, Ching-shih (Louis) wrote:
> >>>> Hi reviewers,
> >>>>
> >>>> What is the review result for this patch? Customer is pushing on this
> >>>> change to merge. TKS for your attention.
> >>>>
> >>>> BR,
> >>>> Louis
> >>>>
> >>>> -----Original Message-----
> >>>> From: Louis Li <Ching-shih.Li@amd.com>
> >>>> Sent: Thursday, November 14, 2019 11:42 AM
> >>>> To: amd-gfx@lists.freedesktop.org
> >>>> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry
> >>>> <Harry.Wentland@amd.com>; Li, Ching-shih (Louis) <Ching-shih.Li@amd.com>
> >>>> Subject: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be
> >>>> successfully detected
> >>>>
> >>>> [Why]
> >>>> External monitor cannot be displayed consistently, if connecting via
> >>>> this Apple dongle (A1621, USB Type-C to HDMI).
> >>>> By experiments, it is confirmed that the dongle needs 200ms at least to
> >>>> be ready for communication, after it sets HPD signal high.
> >>>>
> >>>> [How]
> >>>> When receiving HPD IRQ, delay 300ms at the beginning of handle_hpd_irq().
> >>>> Then run the original procedure.
> >>>> With this patch applied, the problem cannot be reproduced.
> >>>> With other dongles, test results are PASS.
> >>>> Test result is PASS to plug in HDMI cable while playing video, and the
> >>>> video is being playing smoothly.
> >>>> Test result is PASS after system resumes from suspend.
> >>>>
> >>>> Signed-off-by: Louis Li <Ching-shih.Li@amd.com>
> >>>
> >>> This is still a NAK from me since the logic hasn't changed from the first
> >>> patch.
> >>>
> >>> Sleeps don't belong in IRQ handlers.
> >>>
> >>> Regards,
> >>> Nicholas Kazlauskas
> >>
> >> Actually, this lives in the low IRQ context which means that it's already
> >> been queued off to a work thread so it's safe to sleep.
> >>
> >> I'm not sure we want a general 300ms sleep (even by experiment you said that
> >> it only needed 200ms) for all connectors.
> >>
> >> Nicholas Kazlauskas
> >>
> > 
> > Yes, it is IRQ context. Safe to call sleep(). Moreover, in current driver,
> > even udelay() is called in wait_for_training_aux_rd_interval() in the flow
> > of handle_hpd_irq().
> > 
> > For 2nd question, of course not all connectors have this behavior.
> > Based on real cases we ever dealt, some dongles like this, or some
> > monitors driven by TCON, have same behavior. And no chance to read
> > out anything to decide if delay is needed. This change does help
> > to have our driver gain better compatibility. Truly this should be
> > problem of dongles/monitors. We are not the only one to
> > workaround such a problem. This change does not hurt other connects,
> > and some other dongles are tested ok, e.g. HP/Huwai dongles, etc.
> > 
> 
> I still don't like this change. It might impact other use cases, such as
> SST-to-MST switching on MST displays.
> 
> Have you checked how Windows deals with this dongle and how the Windows
> team solved this? Have you checked how other drivers (such as i915) deal
> with this dongle?
> 
> Have you checked whether you can pass DP compliance with this change?
> 
> Harry
> 

Good points. MST and DP compliance are not verified yet.

For Windows cases, same solution was implemented. As I know, it goes to
point release (PR) instead of main line. Company N. has similar solution
to workaround such a problem. For i915, the solution seems to be different.

Will this change be accepted if it can pass MST and compilance?

Louis

> >>>
> >>>> ---
> >>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
> >>>>   1 file changed, 5 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 0aef92b7c037..5b844b6a5a59 100644
> >>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>>> @@ -1025,6 +1025,11 @@ static void handle_hpd_irq(void *param)
> >>>>       struct drm_device *dev = connector->dev;
> >>>>       enum dc_connection_type new_connection_type = dc_connection_none;
> >>>> +    /* Some monitors/dongles need around 200ms to be ready for
> >>>> communication
> >>>> +     * after those devices drive HPD signal high.
> >>>> +     */
> >>>> +    msleep(300);
> >>>> +
> >>>>       /* In case of failure or MST no need to update connector status or
> >>>> notify the OS
> >>>>        * since (for MST case) MST does this in it's own context.
> >>>>        */
> >>>> -- 
> >>>> 2.21.0
> >>>>
> >>>
> >>> _______________________________________________
> >>> 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: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[Louis Li]

On Fri, Nov 22, 2019 at 10:31:19AM -0500, Harry Wentland wrote:
> 
> 
> On 2019-11-22 1:33 a.m., Louis Li wrote:
> > On Thu, Nov 21, 2019 at 08:47:50AM -0500, Kazlauskas, Nicholas wrote:
> >> On 2019-11-21 8:40 a.m., Kazlauskas, Nicholas wrote:
> >>> On 2019-11-21 3:31 a.m., Li, Ching-shih (Louis) wrote:
> >>>> Hi reviewers,
> >>>>
> >>>> What is the review result for this patch? Customer is pushing on this
> >>>> change to merge. TKS for your attention.
> >>>>
> >>>> BR,
> >>>> Louis
> >>>>
> >>>> -----Original Message-----
> >>>> From: Louis Li <Ching-shih.Li@amd.com>
> >>>> Sent: Thursday, November 14, 2019 11:42 AM
> >>>> To: amd-gfx@lists.freedesktop.org
> >>>> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry
> >>>> <Harry.Wentland@amd.com>; Li, Ching-shih (Louis) <Ching-shih.Li@amd.com>
> >>>> Subject: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be
> >>>> successfully detected
> >>>>
> >>>> [Why]
> >>>> External monitor cannot be displayed consistently, if connecting via
> >>>> this Apple dongle (A1621, USB Type-C to HDMI).
> >>>> By experiments, it is confirmed that the dongle needs 200ms at least to
> >>>> be ready for communication, after it sets HPD signal high.
> >>>>
> >>>> [How]
> >>>> When receiving HPD IRQ, delay 300ms at the beginning of handle_hpd_irq().
> >>>> Then run the original procedure.
> >>>> With this patch applied, the problem cannot be reproduced.
> >>>> With other dongles, test results are PASS.
> >>>> Test result is PASS to plug in HDMI cable while playing video, and the
> >>>> video is being playing smoothly.
> >>>> Test result is PASS after system resumes from suspend.
> >>>>
> >>>> Signed-off-by: Louis Li <Ching-shih.Li@amd.com>
> >>>
> >>> This is still a NAK from me since the logic hasn't changed from the first
> >>> patch.
> >>>
> >>> Sleeps don't belong in IRQ handlers.
> >>>
> >>> Regards,
> >>> Nicholas Kazlauskas
> >>
> >> Actually, this lives in the low IRQ context which means that it's already
> >> been queued off to a work thread so it's safe to sleep.
> >>
> >> I'm not sure we want a general 300ms sleep (even by experiment you said that
> >> it only needed 200ms) for all connectors.
> >>
> >> Nicholas Kazlauskas
> >>
> > 
> > Yes, it is IRQ context. Safe to call sleep(). Moreover, in current driver,
> > even udelay() is called in wait_for_training_aux_rd_interval() in the flow
> > of handle_hpd_irq().
> > 
> > For 2nd question, of course not all connectors have this behavior.
> > Based on real cases we ever dealt, some dongles like this, or some
> > monitors driven by TCON, have same behavior. And no chance to read
> > out anything to decide if delay is needed. This change does help
> > to have our driver gain better compatibility. Truly this should be
> > problem of dongles/monitors. We are not the only one to
> > workaround such a problem. This change does not hurt other connects,
> > and some other dongles are tested ok, e.g. HP/Huwai dongles, etc.
> > 
> 
> I still don't like this change. It might impact other use cases, such as
> SST-to-MST switching on MST displays.
> 
> Have you checked how Windows deals with this dongle and how the Windows
> team solved this? Have you checked how other drivers (such as i915) deal
> with this dongle?
> 
> Have you checked whether you can pass DP compliance with this change?
> 
> Harry
> 

Good points. MST and DP compliance are not verified yet.

For Windows cases, same solution was implemented. As I know, it goes to
point release (PR) instead of main line. Company N. has similar solution
to workaround such a problem. For i915, the solution seems to be different.

Will this change be accepted if it can pass MST and compilance?

Louis

> >>>
> >>>> ---
> >>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
> >>>>   1 file changed, 5 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 0aef92b7c037..5b844b6a5a59 100644
> >>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>>> @@ -1025,6 +1025,11 @@ static void handle_hpd_irq(void *param)
> >>>>       struct drm_device *dev = connector->dev;
> >>>>       enum dc_connection_type new_connection_type = dc_connection_none;
> >>>> +    /* Some monitors/dongles need around 200ms to be ready for
> >>>> communication
> >>>> +     * after those devices drive HPD signal high.
> >>>> +     */
> >>>> +    msleep(300);
> >>>> +
> >>>>       /* In case of failure or MST no need to update connector status or
> >>>> notify the OS
> >>>>        * since (for MST case) MST does this in it's own context.
> >>>>        */
> >>>> -- 
> >>>> 2.21.0
> >>>>
> >>>
> >>> _______________________________________________
> >>> 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: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[Harry Wentland]

On 2019-11-25 4:49 a.m., Louis Li wrote:
> On Fri, Nov 22, 2019 at 10:31:19AM -0500, Harry Wentland wrote:
>>
>>
>> On 2019-11-22 1:33 a.m., Louis Li wrote:
>>> On Thu, Nov 21, 2019 at 08:47:50AM -0500, Kazlauskas, Nicholas wrote:
>>>> On 2019-11-21 8:40 a.m., Kazlauskas, Nicholas wrote:
>>>>> On 2019-11-21 3:31 a.m., Li, Ching-shih (Louis) wrote:
>>>>>> Hi reviewers,
>>>>>>
>>>>>> What is the review result for this patch? Customer is pushing on this
>>>>>> change to merge. TKS for your attention.
>>>>>>
>>>>>> BR,
>>>>>> Louis
>>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Louis Li <Ching-shih.Li@amd.com>
>>>>>> Sent: Thursday, November 14, 2019 11:42 AM
>>>>>> To: amd-gfx@lists.freedesktop.org
>>>>>> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry
>>>>>> <Harry.Wentland@amd.com>; Li, Ching-shih (Louis) <Ching-shih.Li@amd.com>
>>>>>> Subject: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be
>>>>>> successfully detected
>>>>>>
>>>>>> [Why]
>>>>>> External monitor cannot be displayed consistently, if connecting via
>>>>>> this Apple dongle (A1621, USB Type-C to HDMI).
>>>>>> By experiments, it is confirmed that the dongle needs 200ms at least to
>>>>>> be ready for communication, after it sets HPD signal high.
>>>>>>
>>>>>> [How]
>>>>>> When receiving HPD IRQ, delay 300ms at the beginning of handle_hpd_irq().
>>>>>> Then run the original procedure.
>>>>>> With this patch applied, the problem cannot be reproduced.
>>>>>> With other dongles, test results are PASS.
>>>>>> Test result is PASS to plug in HDMI cable while playing video, and the
>>>>>> video is being playing smoothly.
>>>>>> Test result is PASS after system resumes from suspend.
>>>>>>
>>>>>> Signed-off-by: Louis Li <Ching-shih.Li@amd.com>
>>>>>
>>>>> This is still a NAK from me since the logic hasn't changed from the first
>>>>> patch.
>>>>>
>>>>> Sleeps don't belong in IRQ handlers.
>>>>>
>>>>> Regards,
>>>>> Nicholas Kazlauskas
>>>>
>>>> Actually, this lives in the low IRQ context which means that it's already
>>>> been queued off to a work thread so it's safe to sleep.
>>>>
>>>> I'm not sure we want a general 300ms sleep (even by experiment you said that
>>>> it only needed 200ms) for all connectors.
>>>>
>>>> Nicholas Kazlauskas
>>>>
>>>
>>> Yes, it is IRQ context. Safe to call sleep(). Moreover, in current driver,
>>> even udelay() is called in wait_for_training_aux_rd_interval() in the flow
>>> of handle_hpd_irq().
>>>
>>> For 2nd question, of course not all connectors have this behavior.
>>> Based on real cases we ever dealt, some dongles like this, or some
>>> monitors driven by TCON, have same behavior. And no chance to read
>>> out anything to decide if delay is needed. This change does help
>>> to have our driver gain better compatibility. Truly this should be
>>> problem of dongles/monitors. We are not the only one to
>>> workaround such a problem. This change does not hurt other connects,
>>> and some other dongles are tested ok, e.g. HP/Huwai dongles, etc.
>>>
>>
>> I still don't like this change. It might impact other use cases, such as
>> SST-to-MST switching on MST displays.
>>
>> Have you checked how Windows deals with this dongle and how the Windows
>> team solved this? Have you checked how other drivers (such as i915) deal
>> with this dongle?
>>
>> Have you checked whether you can pass DP compliance with this change?
>>
>> Harry
>>
> 
> Good points. MST and DP compliance are not verified yet.
> 
> For Windows cases, same solution was implemented. As I know, it goes to
> point release (PR) instead of main line. Company N. has similar solution
> to workaround such a problem. For i915, the solution seems to be different.
> 
> Will this change be accepted if it can pass MST and compilance?
> 

Same as for Windows I'm not convinced that this change should go into
mainline. If the customer needs a workaround we can always provide it on
a customer branch.

Harry

> Louis
> 
>>>>>
>>>>>> ---
>>>>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
>>>>>>   1 file changed, 5 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 0aef92b7c037..5b844b6a5a59 100644
>>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>> @@ -1025,6 +1025,11 @@ static void handle_hpd_irq(void *param)
>>>>>>       struct drm_device *dev = connector->dev;
>>>>>>       enum dc_connection_type new_connection_type = dc_connection_none;
>>>>>> +    /* Some monitors/dongles need around 200ms to be ready for
>>>>>> communication
>>>>>> +     * after those devices drive HPD signal high.
>>>>>> +     */
>>>>>> +    msleep(300);
>>>>>> +
>>>>>>       /* In case of failure or MST no need to update connector status or
>>>>>> notify the OS
>>>>>>        * since (for MST case) MST does this in it's own context.
>>>>>>        */
>>>>>> -- 
>>>>>> 2.21.0
>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> 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: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[Harry Wentland]

On 2019-11-25 4:49 a.m., Louis Li wrote:
> On Fri, Nov 22, 2019 at 10:31:19AM -0500, Harry Wentland wrote:
>>
>>
>> On 2019-11-22 1:33 a.m., Louis Li wrote:
>>> On Thu, Nov 21, 2019 at 08:47:50AM -0500, Kazlauskas, Nicholas wrote:
>>>> On 2019-11-21 8:40 a.m., Kazlauskas, Nicholas wrote:
>>>>> On 2019-11-21 3:31 a.m., Li, Ching-shih (Louis) wrote:
>>>>>> Hi reviewers,
>>>>>>
>>>>>> What is the review result for this patch? Customer is pushing on this
>>>>>> change to merge. TKS for your attention.
>>>>>>
>>>>>> BR,
>>>>>> Louis
>>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Louis Li <Ching-shih.Li@amd.com>
>>>>>> Sent: Thursday, November 14, 2019 11:42 AM
>>>>>> To: amd-gfx@lists.freedesktop.org
>>>>>> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry
>>>>>> <Harry.Wentland@amd.com>; Li, Ching-shih (Louis) <Ching-shih.Li@amd.com>
>>>>>> Subject: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be
>>>>>> successfully detected
>>>>>>
>>>>>> [Why]
>>>>>> External monitor cannot be displayed consistently, if connecting via
>>>>>> this Apple dongle (A1621, USB Type-C to HDMI).
>>>>>> By experiments, it is confirmed that the dongle needs 200ms at least to
>>>>>> be ready for communication, after it sets HPD signal high.
>>>>>>
>>>>>> [How]
>>>>>> When receiving HPD IRQ, delay 300ms at the beginning of handle_hpd_irq().
>>>>>> Then run the original procedure.
>>>>>> With this patch applied, the problem cannot be reproduced.
>>>>>> With other dongles, test results are PASS.
>>>>>> Test result is PASS to plug in HDMI cable while playing video, and the
>>>>>> video is being playing smoothly.
>>>>>> Test result is PASS after system resumes from suspend.
>>>>>>
>>>>>> Signed-off-by: Louis Li <Ching-shih.Li@amd.com>
>>>>>
>>>>> This is still a NAK from me since the logic hasn't changed from the first
>>>>> patch.
>>>>>
>>>>> Sleeps don't belong in IRQ handlers.
>>>>>
>>>>> Regards,
>>>>> Nicholas Kazlauskas
>>>>
>>>> Actually, this lives in the low IRQ context which means that it's already
>>>> been queued off to a work thread so it's safe to sleep.
>>>>
>>>> I'm not sure we want a general 300ms sleep (even by experiment you said that
>>>> it only needed 200ms) for all connectors.
>>>>
>>>> Nicholas Kazlauskas
>>>>
>>>
>>> Yes, it is IRQ context. Safe to call sleep(). Moreover, in current driver,
>>> even udelay() is called in wait_for_training_aux_rd_interval() in the flow
>>> of handle_hpd_irq().
>>>
>>> For 2nd question, of course not all connectors have this behavior.
>>> Based on real cases we ever dealt, some dongles like this, or some
>>> monitors driven by TCON, have same behavior. And no chance to read
>>> out anything to decide if delay is needed. This change does help
>>> to have our driver gain better compatibility. Truly this should be
>>> problem of dongles/monitors. We are not the only one to
>>> workaround such a problem. This change does not hurt other connects,
>>> and some other dongles are tested ok, e.g. HP/Huwai dongles, etc.
>>>
>>
>> I still don't like this change. It might impact other use cases, such as
>> SST-to-MST switching on MST displays.
>>
>> Have you checked how Windows deals with this dongle and how the Windows
>> team solved this? Have you checked how other drivers (such as i915) deal
>> with this dongle?
>>
>> Have you checked whether you can pass DP compliance with this change?
>>
>> Harry
>>
> 
> Good points. MST and DP compliance are not verified yet.
> 
> For Windows cases, same solution was implemented. As I know, it goes to
> point release (PR) instead of main line. Company N. has similar solution
> to workaround such a problem. For i915, the solution seems to be different.
> 
> Will this change be accepted if it can pass MST and compilance?
> 

Same as for Windows I'm not convinced that this change should go into
mainline. If the customer needs a workaround we can always provide it on
a customer branch.

Harry

> Louis
> 
>>>>>
>>>>>> ---
>>>>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
>>>>>>   1 file changed, 5 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 0aef92b7c037..5b844b6a5a59 100644
>>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>>> @@ -1025,6 +1025,11 @@ static void handle_hpd_irq(void *param)
>>>>>>       struct drm_device *dev = connector->dev;
>>>>>>       enum dc_connection_type new_connection_type = dc_connection_none;
>>>>>> +    /* Some monitors/dongles need around 200ms to be ready for
>>>>>> communication
>>>>>> +     * after those devices drive HPD signal high.
>>>>>> +     */
>>>>>> +    msleep(300);
>>>>>> +
>>>>>>       /* In case of failure or MST no need to update connector status or
>>>>>> notify the OS
>>>>>>        * since (for MST case) MST does this in it's own context.
>>>>>>        */
>>>>>> -- 
>>>>>> 2.21.0
>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> 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: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[Louis Li]

On Mon, Nov 25, 2019 at 09:24:56AM -0500, Harry Wentland wrote:
> 
> 
> On 2019-11-25 4:49 a.m., Louis Li wrote:
> > On Fri, Nov 22, 2019 at 10:31:19AM -0500, Harry Wentland wrote:
> >>
> >>
> >> On 2019-11-22 1:33 a.m., Louis Li wrote:
> >>> On Thu, Nov 21, 2019 at 08:47:50AM -0500, Kazlauskas, Nicholas wrote:
> >>>> On 2019-11-21 8:40 a.m., Kazlauskas, Nicholas wrote:
> >>>>> On 2019-11-21 3:31 a.m., Li, Ching-shih (Louis) wrote:
> >>>>>> Hi reviewers,
> >>>>>>
> >>>>>> What is the review result for this patch? Customer is pushing on this
> >>>>>> change to merge. TKS for your attention.
> >>>>>>
> >>>>>> BR,
> >>>>>> Louis
> >>>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Louis Li <Ching-shih.Li@amd.com>
> >>>>>> Sent: Thursday, November 14, 2019 11:42 AM
> >>>>>> To: amd-gfx@lists.freedesktop.org
> >>>>>> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry
> >>>>>> <Harry.Wentland@amd.com>; Li, Ching-shih (Louis) <Ching-shih.Li@amd.com>
> >>>>>> Subject: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be
> >>>>>> successfully detected
> >>>>>>
> >>>>>> [Why]
> >>>>>> External monitor cannot be displayed consistently, if connecting via
> >>>>>> this Apple dongle (A1621, USB Type-C to HDMI).
> >>>>>> By experiments, it is confirmed that the dongle needs 200ms at least to
> >>>>>> be ready for communication, after it sets HPD signal high.
> >>>>>>
> >>>>>> [How]
> >>>>>> When receiving HPD IRQ, delay 300ms at the beginning of handle_hpd_irq().
> >>>>>> Then run the original procedure.
> >>>>>> With this patch applied, the problem cannot be reproduced.
> >>>>>> With other dongles, test results are PASS.
> >>>>>> Test result is PASS to plug in HDMI cable while playing video, and the
> >>>>>> video is being playing smoothly.
> >>>>>> Test result is PASS after system resumes from suspend.
> >>>>>>
> >>>>>> Signed-off-by: Louis Li <Ching-shih.Li@amd.com>
> >>>>>
> >>>>> This is still a NAK from me since the logic hasn't changed from the first
> >>>>> patch.
> >>>>>
> >>>>> Sleeps don't belong in IRQ handlers.
> >>>>>
> >>>>> Regards,
> >>>>> Nicholas Kazlauskas
> >>>>
> >>>> Actually, this lives in the low IRQ context which means that it's already
> >>>> been queued off to a work thread so it's safe to sleep.
> >>>>
> >>>> I'm not sure we want a general 300ms sleep (even by experiment you said that
> >>>> it only needed 200ms) for all connectors.
> >>>>
> >>>> Nicholas Kazlauskas
> >>>>
> >>>
> >>> Yes, it is IRQ context. Safe to call sleep(). Moreover, in current driver,
> >>> even udelay() is called in wait_for_training_aux_rd_interval() in the flow
> >>> of handle_hpd_irq().
> >>>
> >>> For 2nd question, of course not all connectors have this behavior.
> >>> Based on real cases we ever dealt, some dongles like this, or some
> >>> monitors driven by TCON, have same behavior. And no chance to read
> >>> out anything to decide if delay is needed. This change does help
> >>> to have our driver gain better compatibility. Truly this should be
> >>> problem of dongles/monitors. We are not the only one to
> >>> workaround such a problem. This change does not hurt other connects,
> >>> and some other dongles are tested ok, e.g. HP/Huwai dongles, etc.
> >>>
> >>
> >> I still don't like this change. It might impact other use cases, such as
> >> SST-to-MST switching on MST displays.
> >>
> >> Have you checked how Windows deals with this dongle and how the Windows
> >> team solved this? Have you checked how other drivers (such as i915) deal
> >> with this dongle?
> >>
> >> Have you checked whether you can pass DP compliance with this change?
> >>
> >> Harry
> >>
> > 
> > Good points. MST and DP compliance are not verified yet.
> > 
> > For Windows cases, same solution was implemented. As I know, it goes to
> > point release (PR) instead of main line. Company N. has similar solution
> > to workaround such a problem. For i915, the solution seems to be different.
> > 
> > Will this change be accepted if it can pass MST and compilance?
> > 
> 
> Same as for Windows I'm not convinced that this change should go into
> mainline. If the customer needs a workaround we can always provide it on
> a customer branch.
> 
> Harry
> 

Understand. Forward this comment to customer. Not sure what response will
be.

> > Louis
> > 
> >>>>>
> >>>>>> ---
> >>>>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
> >>>>>>   1 file changed, 5 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 0aef92b7c037..5b844b6a5a59 100644
> >>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>>>>> @@ -1025,6 +1025,11 @@ static void handle_hpd_irq(void *param)
> >>>>>>       struct drm_device *dev = connector->dev;
> >>>>>>       enum dc_connection_type new_connection_type = dc_connection_none;
> >>>>>> +    /* Some monitors/dongles need around 200ms to be ready for
> >>>>>> communication
> >>>>>> +     * after those devices drive HPD signal high.
> >>>>>> +     */
> >>>>>> +    msleep(300);
> >>>>>> +
> >>>>>>       /* In case of failure or MST no need to update connector status or
> >>>>>> notify the OS
> >>>>>>        * since (for MST case) MST does this in it's own context.
> >>>>>>        */
> >>>>>> -- 
> >>>>>> 2.21.0
> >>>>>>
> >>>>>
> >>>>> _______________________________________________
> >>>>> 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: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be successfully detected

[Louis Li]

On Mon, Nov 25, 2019 at 09:24:56AM -0500, Harry Wentland wrote:
> 
> 
> On 2019-11-25 4:49 a.m., Louis Li wrote:
> > On Fri, Nov 22, 2019 at 10:31:19AM -0500, Harry Wentland wrote:
> >>
> >>
> >> On 2019-11-22 1:33 a.m., Louis Li wrote:
> >>> On Thu, Nov 21, 2019 at 08:47:50AM -0500, Kazlauskas, Nicholas wrote:
> >>>> On 2019-11-21 8:40 a.m., Kazlauskas, Nicholas wrote:
> >>>>> On 2019-11-21 3:31 a.m., Li, Ching-shih (Louis) wrote:
> >>>>>> Hi reviewers,
> >>>>>>
> >>>>>> What is the review result for this patch? Customer is pushing on this
> >>>>>> change to merge. TKS for your attention.
> >>>>>>
> >>>>>> BR,
> >>>>>> Louis
> >>>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Louis Li <Ching-shih.Li@amd.com>
> >>>>>> Sent: Thursday, November 14, 2019 11:42 AM
> >>>>>> To: amd-gfx@lists.freedesktop.org
> >>>>>> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry
> >>>>>> <Harry.Wentland@amd.com>; Li, Ching-shih (Louis) <Ching-shih.Li@amd.com>
> >>>>>> Subject: [PATCH v2] drm/amd/display: Fix Apple dongle cannot be
> >>>>>> successfully detected
> >>>>>>
> >>>>>> [Why]
> >>>>>> External monitor cannot be displayed consistently, if connecting via
> >>>>>> this Apple dongle (A1621, USB Type-C to HDMI).
> >>>>>> By experiments, it is confirmed that the dongle needs 200ms at least to
> >>>>>> be ready for communication, after it sets HPD signal high.
> >>>>>>
> >>>>>> [How]
> >>>>>> When receiving HPD IRQ, delay 300ms at the beginning of handle_hpd_irq().
> >>>>>> Then run the original procedure.
> >>>>>> With this patch applied, the problem cannot be reproduced.
> >>>>>> With other dongles, test results are PASS.
> >>>>>> Test result is PASS to plug in HDMI cable while playing video, and the
> >>>>>> video is being playing smoothly.
> >>>>>> Test result is PASS after system resumes from suspend.
> >>>>>>
> >>>>>> Signed-off-by: Louis Li <Ching-shih.Li@amd.com>
> >>>>>
> >>>>> This is still a NAK from me since the logic hasn't changed from the first
> >>>>> patch.
> >>>>>
> >>>>> Sleeps don't belong in IRQ handlers.
> >>>>>
> >>>>> Regards,
> >>>>> Nicholas Kazlauskas
> >>>>
> >>>> Actually, this lives in the low IRQ context which means that it's already
> >>>> been queued off to a work thread so it's safe to sleep.
> >>>>
> >>>> I'm not sure we want a general 300ms sleep (even by experiment you said that
> >>>> it only needed 200ms) for all connectors.
> >>>>
> >>>> Nicholas Kazlauskas
> >>>>
> >>>
> >>> Yes, it is IRQ context. Safe to call sleep(). Moreover, in current driver,
> >>> even udelay() is called in wait_for_training_aux_rd_interval() in the flow
> >>> of handle_hpd_irq().
> >>>
> >>> For 2nd question, of course not all connectors have this behavior.
> >>> Based on real cases we ever dealt, some dongles like this, or some
> >>> monitors driven by TCON, have same behavior. And no chance to read
> >>> out anything to decide if delay is needed. This change does help
> >>> to have our driver gain better compatibility. Truly this should be
> >>> problem of dongles/monitors. We are not the only one to
> >>> workaround such a problem. This change does not hurt other connects,
> >>> and some other dongles are tested ok, e.g. HP/Huwai dongles, etc.
> >>>
> >>
> >> I still don't like this change. It might impact other use cases, such as
> >> SST-to-MST switching on MST displays.
> >>
> >> Have you checked how Windows deals with this dongle and how the Windows
> >> team solved this? Have you checked how other drivers (such as i915) deal
> >> with this dongle?
> >>
> >> Have you checked whether you can pass DP compliance with this change?
> >>
> >> Harry
> >>
> > 
> > Good points. MST and DP compliance are not verified yet.
> > 
> > For Windows cases, same solution was implemented. As I know, it goes to
> > point release (PR) instead of main line. Company N. has similar solution
> > to workaround such a problem. For i915, the solution seems to be different.
> > 
> > Will this change be accepted if it can pass MST and compilance?
> > 
> 
> Same as for Windows I'm not convinced that this change should go into
> mainline. If the customer needs a workaround we can always provide it on
> a customer branch.
> 
> Harry
> 

Understand. Forward this comment to customer. Not sure what response will
be.

> > Louis
> > 
> >>>>>
> >>>>>> ---
> >>>>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
> >>>>>>   1 file changed, 5 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 0aef92b7c037..5b844b6a5a59 100644
> >>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>>>>> @@ -1025,6 +1025,11 @@ static void handle_hpd_irq(void *param)
> >>>>>>       struct drm_device *dev = connector->dev;
> >>>>>>       enum dc_connection_type new_connection_type = dc_connection_none;
> >>>>>> +    /* Some monitors/dongles need around 200ms to be ready for
> >>>>>> communication
> >>>>>> +     * after those devices drive HPD signal high.
> >>>>>> +     */
> >>>>>> +    msleep(300);
> >>>>>> +
> >>>>>>       /* In case of failure or MST no need to update connector status or
> >>>>>> notify the OS
> >>>>>>        * since (for MST case) MST does this in it's own context.
> >>>>>>        */
> >>>>>> -- 
> >>>>>> 2.21.0
> >>>>>>
> >>>>>
> >>>>> _______________________________________________
> >>>>> 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