Re: [PATCH v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle

[Kuogee Hsieh]

On 4/14/2022 4:34 PM, Dmitry Baryshkov wrote:
> On 15/04/2022 01:06, Kuogee Hsieh wrote:
> > On 4/8/2022 4:59 PM, Dmitry Baryshkov wrote:
> > > On Fri, 8 Apr 2022 at 23:30, Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx> 
> > > wrote:
> > > > On 4/8/2022 5:27 AM, Dmitry Baryshkov wrote:
> > > > > On 07/04/2022 00:28, Kuogee Hsieh wrote:
> > > > > > dp_hpd_plug_handle() is responsible for setting up main link and 
> > > > > > send
> > > > > > uevent to notify user space framework to start video stream. 
> > > > > > Similarly,
> > > > > > dp_hdp_unplug_handle is responsible to send uevent to notify user 
> > > > > > space
> > > > > > framework to stop video stream and then tear down main link.
> > > > > > However there are rare cases, such as in the middle of system
> > > > > > suspending,
> > > > > > that uevent could not be delivered to user space framework. 
> > > > > > Therefore
> > > > > > some kind of recover mechanism armed by timer need to be in place 
> > > > > > in the
> > > > > > case of user space framework does not respond to uevent.
> > > > > Hmm, how does userpsace 'respond' to the uevent? The driver should
> > > > > send hotplug notifications to userspace, but it must not expect any
> > > > > particular reaction. The userspace might be as simple, as fbdev
> > > > > emulation, but the driver still should function correctly.
> > > > yes, driver is function correctly by setting up main link. but it does
> > > > not know which resolution to display.
> > > >
> > > > It send hotplug notification through uevent to framework after main 
> > > > link
> > > > is ready.
> > > >
> > > > Framework  is responsible to set up MDP timing engine to start 
> > > > video stream.
> > > >
> > > >
> > > > However it does not know which
> > > It's of no concern to the driver. It is completely the userspace
> > > problem. After resuming, it should reread available video output
> > > properties. The display could have been changed while the system is
> > > suspended.
> > >  From your description I still do not understand why you need the
> > > 'recovery' mechanism.
> >
> > I mean "recovery" means  dp driver may leave stay at link ready 
> > mistakenly after dongle unplugged due to missing framework action to 
> > tear down main link so that next dongle plug in will not work.
> >
> > Currently it was armed with timeout function and it will fired if 
> > framework did not call msm_dp_display_disable() after 5 second.
>
> framework = userspace?
>
> Is my understanding correct? Currently DP driver sends a notification 
> to userspace that DP is unplugged, waits for userspace to disable DP 
> output, and only after that it will shutdown the link. Is this a 
> correct description?
Yes, you are correct, connection need to be tear down from top to bottom.
> If so, then yes, your change is correct. I mean that the kernel 
> shouldn't wait for clients to stop using video pipeline if the sink 
> gets unplugged. Instead it should tear the video stream down and let 
> the DRM client cope with that.
>
> I'm not sure how should the driver react if the client doesn't disable 
> the output, but then the sink gets reattached?

I do not know that either.

But it should not happen as long as framework response to uevent.

> And a bit more interesting question: how should the driver behave if 
> the new sink is not compatible with the existing video stream.

When dongle plug/replug in, driver will always read sink's edid.

Therefore resolution database should be updated accordingly.

> > Anyway, we think this approach is not good. therefore it is replaced 
> > with  below patch.
> >
> > drm/msm/dp: tear down main link at unplug handle immediately