[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 at quicinc.com> 
>>> 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
>
>