[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 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
>>>>
>>>
>>> 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 at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>