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