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@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? > 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@xxxxxxxxxxxxxxxxxxxxx >>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >>>> _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
