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