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

Tue Nov 26 08:51:25 UTC 2019

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 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
> 

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