Does this seem like I'm on the right track so far? JFYI - I've been busy trying to write up some patches for this, but there's definitely a lot of code to go through and change. On Wed, 2022-03-16 at 16:28 -0400, Lyude Paul wrote: > On Wed, 2022-03-16 at 13:01 +0200, Ville Syrjälä wrote: > > On Mon, Mar 14, 2022 at 06:16:36PM -0400, Lyude Paul wrote: > > > Hi! First a little bit of background: I've recently been trying to get > > > rid > > > of > > > all of the non-atomic payload bandwidth management code in the MST > > > helpers > > > in > > > order to make it easier to implement DSC and fallback link rate > > > retraining > > > support down the line. Currently bandwidth information is stored in two > > > places, partially in the MST atomic state and partially in the mst > > > manager's > > > payload table (which exists outside of the atomic state and has its own > > > locking). The portions in the atomic state are used to try to determine > > > if > > > a > > > given display configuration can fit within the given bandwidth > > > limitations > > > during the atomic check phase, and are implemented through the use of > > > private > > > state objects. > > > > > > My current goal has been to move as much of this as possible over to the > > > atomic state and entirely get rid of the payload table along with it's > > > locks. > > > My main reason for doing this is that it both decomplicates things quite > > > a > > > bit, and I'm really also hoping that getting rid of that payload code > > > will > > > make it clearer to others how it works - and stop the influx of bandaid > > > patches (e.g. adding more and more special cases to MST to fix poorly > > > understood issues being hit in one specific driver and nowhere else) > > > that > > > I've > > > had to spend way more time then I'd like trying to investigate and > > > review. > > > > > > So, the actual problem: following a conversation with Daniel Vetter > > > today > > > I've > > > gotten the impression that private modesetting objects are basically > > > just > > > broken with parallel modesets? I'm still wrapping my head around all of > > > this > > > honestly, but from what I've gathered: CRTC atomic infra knows how to do > > > waits > > > in the proper places for when other CRTCs need to be waited on to > > > continue > > > a > > > modeset, but there's no such tracking with private objects. If I > > > understand > > > this correctly, that means that even if two CRTC modesets require > > > pulling > > > in > > > the same private object state for the MST mgr: we're only provided a > > > guarantee > > > that the atomic checks pulling in that private object state won't > > > concurrently. But when it comes to commits, it doesn't sound like > > > there's > > > any > > > actual tracking for this and as such - two CRTC modesets which have both > > > pulled in the MST private state object are not prevented from running > > > concurrently. > > > > > > This unfortunately throws an enormous wrench into the MST atomic > > > conversion > > > I've been working on - as I was under the understanding while writing > > > the > > > code > > > for this that all objects in an atomic state are blocked from being used > > > in > > > any new atomic commits (not checks, as parallel checks should be fine in > > > my > > > case) until there's no commits active with said object pulled into the > > > atomic > > > state. I certainly am not aware of any way parallel modesetting could > > > actually > > > be supported on MST, so it's not really a feature we want to deal with > > > at > > > all > > > besides stopping it from happening. This also unfortunately means that > > > the > > > current atomic modesetting code we have for MST is potentially broken, > > > although I assume we've never hit any real world issues with it because > > > of > > > the > > > non-atomic locking we currently have for the payload table. > > > > > > So, Daniel had mentioned that supposedly you've been dealing with > > > similar > > > issues with VC4 and might have already made progress on coming up with > > > ways to > > > deal with it. If this is all correct, I'd definitely appreciate being > > > able > > > to > > > take a look at your work on this to see how I can help move things > > > forward. > > > I've got a WIP of my atomic only MST branch as well: > > > > > > https://gitlab.freedesktop.org/lyudess/linux/-/commits/wip/mst-atomic-only-v1 > > > > > > However it's very certainly broken right now (it compiles and I had > > > thought it > > > worked already, but I realized I totally forgot to come up with a way of > > > doing > > > bookkeeping for VC start slots atomically - which is what led me down > > > this > > > current rabbit hole), but it should at least give a general idea of what > > > I'm > > > trying to do. > > > > > > Anyway, let me know what you think. > > > > For MST in particular parallel modeset on the same physical link sounds > > pretty crazy to me. Trying to make sure everything happens in the right > > order would not be pleasant. I think a simple solution would be just to > > add all the crtcs on the affected link to the state and call it a day. > > JFYI I definitely don't have any kind of plan to try parallel modesetting > with > MST, I think it'd be near impossible to actually get working correctly for > pretty little benefit :). I was just not entirely sure of the work that > would > be required to get private objects to do the right thing here in parallel > modesets (e.g. make sure we wait on all CRTC commits like you mentioned). > > Anyway - I looked at the code for this the other day and a solution that > seems > pretty reasonable for this to me would be to add a hook for DRM private > objects which provides drivers a spot to inform the DRM core what > drm_crtc_commits need to be waited on before starting a modeset. I should > have > some patches on the list soon so folks can tell me if what I'm doing looks > sensible or not :). > > > > > i915 already does that on modern platforms actually because the > > hardware architecture kinda needs it. Although we could perhaps > > optimize it a bit to skip it in some cases, but not sure the > > extra complexity would really be justified. > > > > In i915 we also serialize *all* modesets by running them on a > > ordered wq (+ explicit flush_workqueue() to serialize non-blocking > > vs. blocking modesets). We did semi-accidentally enable parallel > > modesets once but I undid that because there was just way too much > > pre-existing code that wasn't even considering the possibility of > > a parallel modeset, and I didn't really feel like reviewing the > > entire codebase to find all of it. > > > -- Cheers, Lyude Paul (she/her) Software Engineer at Red Hat
