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

[Lyude Paul]

OK so - this has become a bit of a larger rabbit hole. I've been putting quite
a bit of work into this and I think I'm starting to make some progress -
although on a different aspect of this issue. After talking with danvet they
realized that we're also potentially not handling encoder stealing with MST
correctly - which we need to do in order to know that we're correctly pulling
in every related crtc/connector into the state - along with avoiding encoder
conflicts if something tries to use a GPU's DP encoder in SST mode while it's
driving other displays in MST mode.

So - it seems this will likely need to be solved first before we can deal with
ensuring that we perform the correct CRTC waits in atomic commits with the MST
helpers. This has been pretty painful to figure out, but I think I'm starting
to make some progress - but I'd really appreciate getting some feedback on
this approach I've came up with so I maybe can skip having to rewrite it
later.

So: to clarify the problem, it boils down to something like this:

State 1:
  * DP-1 (hosts MST topology, so is disconnected + no encoder)
    * MST topology
      * DP-2 (has display)
      * DP-3 (has display)
  (In hardware)
  * drm_encoder A drives:
    * DP-2
    * DP-3
  (In software)
  * drm_encoder A unused
  * Fake MST drm_encoder B -> DP-2
  * Fake MST drm_encoder C -> DP-3

Problems:
  * DP-1 gets disconnected, MST topology disappears
  * We disable maybe 1 display
  * DP-1 is disconnected, suddenly replaced with SST display
  * Driver tries to assign drm_encoder A to new DP-1 display
  *** Error! drm_encoder A is still driving fake encoders B and C ***


I'm not sure if the exact above example would actually happen - you
might need to do some tricks to get it into such a state. But you get
the general idea - there's missing coverage with how we handle encoder
conflicts when it comes to encoders that aren't directly handling CRTCs.
If we can fix this, I think we should be able to reliably figure out
every CRTC involved in modesets like this - and ensure that nonblocking
modesets come up with the right CRTC dependencies.

My current idea for handling this is as follows:
  * Add the following fields to drm_connector_state:
    * reserved_encoder → an encoder that is "reserved" by the connector,
      but is not directly attached to a monitor. Note reserved
      connectors cannot have CRTCs attached to them. When a connector
      has no more CRTCs reserved, it should lose it's reserved encoder.
    * dependent_crtcs → a bitmask of CRTCs that depend on this
      connector's state, but are not directly attached to it.
  * Add the following fields to drm_crtc_state:
    * connector_dependency → a connector whose state this CRTC relies
      on, but is not directly attached to. This connector must be pulled
      into the atomic state whenever this CRTC requires a modeset.

The reason for adding all of these fields to drm_connector_state and
drm_crtc_state is because I don't think it's possible for us to rely on
a particular private object being in all atomic states - so we need a
way for the DRM core to be able to understand these object relationships
on it's own and reference them from any type of atomic state change so
that we can pull in dependent CRTCs as needed.

>From there, we'd just:
  * Add some functions to handle these new fields, something like:
    * drm_atomic_reserve_crtc_for_connector(crtc, encoder, conn_state)
    * drm_atomic_release_crtc_from_connector(crtc, conn_state)
  * Teach the various DRM core functions how to handle these new fields

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