Messages in this thread | | | Date | Tue, 12 Mar 2024 10:39:46 -0700 | Subject | Re: [PATCH] drm/msm/dp: move link_ready out of HPD event thread | From | 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.
| |