From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 030C5C00140 for ; Wed, 10 Aug 2022 22:13:30 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233073AbiHJWN2 (ORCPT ); Wed, 10 Aug 2022 18:13:28 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45930 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230163AbiHJWN0 (ORCPT ); Wed, 10 Aug 2022 18:13:26 -0400 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 26108832DC for ; Wed, 10 Aug 2022 15:13:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1660169604; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=e2F3QmD0SrLKIJ8LZX9kCYhDOznEePDhSMJ1nhI5zKw=; b=TigBFT6d0E2oM7nEUYdIVmk1Y/KdCyf8ux8+maMiWEyh3y62+IEAAESkL4dDgMUVoMCaBA KcC9X0Upslg3eX+2uUyXaCNiGOm+De5C2YL/s1fPAkGWrrQx8XLU33WmqLV922nbZPiF/5 Z4l7LQRyug6cgkwSvLXt2QGXUvru6vo= Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-319-tn51OrTeNxuLHrVwq4UwvQ-1; Wed, 10 Aug 2022 18:13:21 -0400 X-MC-Unique: tn51OrTeNxuLHrVwq4UwvQ-1 Received: by mail-qk1-f198.google.com with SMTP id m17-20020a05620a291100b006b6590830feso13617564qkp.13 for ; Wed, 10 Aug 2022 15:13:21 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:user-agent:organization :references:in-reply-to:date:cc:to:from:subject:message-id :x-gm-message-state:from:to:cc; bh=e2F3QmD0SrLKIJ8LZX9kCYhDOznEePDhSMJ1nhI5zKw=; b=XOkh7ysxxjJoTGQ3RiLJOra2OocafW8qrx+e38KAg4hxlek2NqupU2piyq2yrUJL5d FVGhCXZwIoX+LgLNHTBl4wnVu6HLIqAZUCFLdBetSOrvJxzf8/n5ZUxwPZeb7hKOEFdC PyXDzj06PoFi6+p0tBJIu4ijXfWvHb1DgeM8SOc/wKXjv6ErHe6OCkNhrGSxk13cpNnO crFA+zh03qgftSUP2GIRazUxV9up4RpDWEZfCgMrF+q1xd420oPmRNFewVoGRyyKIbkF oussIG3wzBeP4du15DzLiJDlkOGosiCc+PZJwPjxrLooYCHBxT3dmubdvy94gf7hn34y OAow== X-Gm-Message-State: ACgBeo2srtVLgCkwlO+5oj4iYsfF8hEs7771G4mRw4lqsB2yqhKTPxQw Esl0iumRdUr465mIH7ufnPB8U+BhWVO497z95E5ba7kPXBnl67urGvtNKZo5N46XNaEghGL/nNF Hgv1veG7xKtkoXVK07ZO4uiih X-Received: by 2002:ad4:5f08:0:b0:476:bf01:ffd8 with SMTP id fo8-20020ad45f08000000b00476bf01ffd8mr25039823qvb.14.1660169601329; Wed, 10 Aug 2022 15:13:21 -0700 (PDT) X-Google-Smtp-Source: AA6agR5N/U457+LKyJIZm5Nt/pvduBY+SmUoBTF6bD0CRA3yXSBptcJbDuSLmXnlIBFmpcZlT5iduQ== X-Received: by 2002:ad4:5f08:0:b0:476:bf01:ffd8 with SMTP id fo8-20020ad45f08000000b00476bf01ffd8mr25039804qvb.14.1660169601042; Wed, 10 Aug 2022 15:13:21 -0700 (PDT) Received: from [192.168.8.138] (pool-100-0-245-4.bstnma.fios.verizon.net. [100.0.245.4]) by smtp.gmail.com with ESMTPSA id br19-20020a05620a461300b006b99975ad62sm750360qkb.48.2022.08.10.15.13.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 10 Aug 2022 15:13:20 -0700 (PDT) Message-ID: Subject: Re: [RESEND RFC 15/18] drm/display/dp_mst: Skip releasing payloads if last connected port isn't connected From: Lyude Paul To: "Lin, Wayne" , "dri-devel@lists.freedesktop.org" , "nouveau@lists.freedesktop.org" , "amd-gfx@lists.freedesktop.org" Cc: Ville =?ISO-8859-1?Q?Syrj=E4l=E4?= , "Zuo, Jerry" , Jani Nikula , Imre Deak , Daniel Vetter , Sean Paul , David Airlie , Daniel Vetter , Thomas Zimmermann , "Lakha, Bhawanpreet" , open list Date: Wed, 10 Aug 2022 18:13:18 -0400 In-Reply-To: References: <20220607192933.1333228-1-lyude@redhat.com> <20220607192933.1333228-16-lyude@redhat.com> <31d47373883e9aabe5bfa7b172e21b84cc6a164d.camel@redhat.com> Organization: Red Hat Inc. Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.42.4 (3.42.4-2.fc35) MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2022-08-10 at 03:28 +0000, Lin, Wayne wrote: > Hi Lyude, > Thanks for your time and sorry for late response! > > It's described in 5.6.1.3 of DP spec 2.0: > "MST branch device, in addition to waiting for the ACK from its immediate > Upstream device, should either wait for the ALLOCATE_PAYLOAD message > transaction with a PBN value equal to 0 from the MST Source device for > de-allocating the time slot assigned to the VC Payload that is routed to the > unplugged DFP or for 2 seconds, whichever occurs first." oooh! Thank you for posting this, I totally missed the bit that says "or for 2 seconds, whichever occurs first." That certainly explains a lot. > > > > commit 3769e4c0af5b ("drm/dp_mst: Avoid to mess up payload table by > > > ports in stale topology") was trying to skip updating payload for a > > > target which is no longer existing in the current topology rooted at > > > mgr->mst_primary. I passed "mgr->mst_primary" to > > > drm_dp_mst_port_downstream_of_branch() previously. > > > Sorry, I might not fully understand the issue you've seen. Could you > > > elaborate on this more please? > > > > > > Thanks! > > > > I will have to double check this since it's been a month, but basically - the idea > > of having the topology references in the first place was to be the one check > > for figuring out whether something's in a topology or not. I've been thinking > > of maybe trying to replace it at some point, but I think we'd want to do it all > > over the helpers instead of just in certain spots. > > > > The other thing I noticed was that when I was rewriting this code, I noticed it > > seemed a lot like we had misunderstood the issue that was causing leaks in > > the first place. The BAD_PARAM we noticed indicates the payload we're > > trying to remove on the other end doesn't exist anymore, meaning the > > branch device in question got rid of any payloads it had active in response to > > the CSN. In testing though I found that payloads would be automatically > > released in situations where the last reachable port was marked as > > disconnected via a previous CSN, but was still reachable otherwise, and not in > > any other situation. This also seemed to match up with the excerpts in the DP > > spec that I found, so I assumed it was probably correct. > > IMHO, the main root cause with the commit 3769e4c0af5b ("drm/dp_mst: Avoid >  to mess up payload table by ports in stale topology") is like what described in the > commit message. The problem I encountered was when I unplugged the primary > mst branch device from the system, upper layer didn't try to  release stale streams > immediately. Instead, it started to gradually release stale streams when I plugged the > mst hub back to the system. In that case, if we didn't do the check to see whether > the current request for deallocating payload is for this time topology instance, > i.e. might be for the stale topology before I unplug, this deallocation will mess up > payload allocation for new topology instance. > > As for the CSN, it's a node broadcast request message and not a path message. > Referring to 2.14.6.1 of DP 2.0 spec: > "If the broadcast message is a node request, only the end devices, DP MST > Source or Sink devices (or DP MST Branch device if Source/Sink are not plugged), > process the request." > IMHO, payload should be controlled by source only, by ALLOCATE_PAYLOAD or > CLEAR_PAYLAOD_ID_TABLE message. > > > > > Also, I think using the DDPS field instead of trying to traverse the topology > > state (which might not have been fully updated yet in response to CSNs) > > might be a slightly better idea since DDPS may end up being updated before > > the port has been removed from our in-memory topology, which is kind of > > Thank you Lyude! Just want to confirm with you the below idea to see if I > understand it correctly. > The flow I thought would be (from Source perspective): > Receive CSN for notifying disconnection event => update physical topology > connection status (e.g. DDPS, put topology krefcount..) => send hotplug event to > userspace => userspace asks deallocating payloads for disconnected stream > sinks =>  put malloc krefcount of disconnected ports/mstbs  => remove ports/mstb > from in-memory topology. > I suppose physical topology connection status is updated before sending hotplug > event to userspace and the in-memory topology still can be referred for stale > connection status before payload deallocation completes, i.e. which will put > malloc krefcount to eventually destroy disconnected devices in topology in-memory. > I mean, ideally, sounds like the topology in-memory should be reliable when > we send ALLOCATE_PAYLOAD as PBN=0. But I understand it definitely is not the > case if we have krefcount leak. mhm, I think you made me realize I'm overthinking this a bit now that I've seen the excerpt you mentioned above, along with the other excerpt about only the end devices being involved. The main reason I originally foresaw an issue with this is because the delay with updating the in-memory topology structure might put us slightly out of sync with the state of the hub on the other end - causing the hub to spit out an error. However - based on the excerpts you mentioned I think what I was seeing was mainly just the 2 second timeout causing things to be released properly - not specific behavior based on the location in the topology of the branch that was just unplugged like I originally assumed. I think in that case it probably does make more sense to go with your fix, so I'll likely drop this and rework the topology checks you had into this. > > Appreciate for your time and help Lyude! > no, thank you for your help! :) There aren't a whole ton of people who are this involved with MST so it's very useful to finally have another pair of eyes looking at all of this. >   -- Cheers, Lyude Paul (she/her) Software Engineer at Red Hat