[PATCH] drm/msm/dp: move link_ready out of HPD event thread

[Abhinav Kumar]

On 3/12/2024 9:59 AM, Johan Hovold wrote:
> On Tue, Mar 12, 2024 at 05:41:23PM +0100, Johan Hovold wrote:
>> On Tue, Mar 12, 2024 at 11:09:11AM +0100, Johan Hovold wrote:
>>> On Fri, Mar 08, 2024 at 01:45:32PM -0800, Abhinav Kumar wrote:
>>
>>>> @@ -466,6 +466,8 @@ static int dp_display_notify_disconnect(struct device *dev)
>>>>   {
>>>>   	struct dp_display_private *dp = dev_get_dp_display_private(dev);
>>>>   
>>>> +	dp->dp_display.link_ready = false;
>>>
>>> As I also pointed out in the other thread, setting link_ready to false
>>> here means that any spurious connect event (during physical disconnect)
>>> will always be processed, something which can currently lead to a leaked
>>> runtime pm reference.
>>>
>>> Wasting some power is of course preferred over crashing the machine, but
>>> please take it into consideration anyway.
>>>
>>> Especially if your intention with this patch was to address the resets
>>> we saw with sc8280xp which are gone since the HPD notify revert (which
>>> fixed the hotplug detect issue that left the bridge in a
>>> half-initialised state).
>>
>> Heh. This is getting ridiculous. I just tried running with this patch
>> and it again breaks hotplug detect in a VT console and in X (where I
>> could enable a reconnected external display by running xrandr twice
>> before).
>>
>> So, please, do not apply this one.
> 
> To make things worse, I indeed also hit the reset when disconnecting
> after such a failed hotplug.
> 
> Johan

Ack, I will hold off till I analyze your issues more which you have 
listed in separate replies. Especially about the spurious connect, I 
believe you are trying to mention that, by adding logs, you are able to 
delay the processing of a connect event to *make* it like a spurious 
one? In case, I got this part wrong, can you pls explain the spurious 
connect scenario again?

A short response on why this change was made is that commit can be 
issued by userspace or the fbdev client. So userspace involvement only 
makes commit happen from a different path. It would be incorrect to 
assume the issues from the earlier bug and the current one are different 
only because there was userspace involvement in that one and not this.

Because in the end, it manifests itself in the same way that 
atomic_enable() did not go through after an atomic_disable() and the 
next atomic_disable() crashes.

Reverting the hpd_notify patch only eliminated some paths but I think 
both you and me agree the issue can still happen and in the very same 
way. So till someone else reports this issue, till HPD is reworked, I 
wanted to do whats possible to avoid this situation. If users are fine 
with what we have, I have no inclination to push this.