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

[Louis Li <ching-li-5C7GfCeVMHo@public.gmane.org>]

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