Parallel modesets and private state objects broken, where to go with MST?

[Lyude Paul]

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