[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 at amd.com>
>> Sent: Thursday, November 14, 2019 11:42 AM
>> To: amd-gfx at lists.freedesktop.org
>> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas at amd.com>; Wentland, 
>> Harry <Harry.Wentland at amd.com>; Li, Ching-shih (Louis) 
>> <Ching-shih.Li at 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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx