drm state readout helpers

[Ville Syrjälä]

On Fri, May 15, 2020 at 03:36:13PM +0200, Daniel Vetter wrote:
> Hi all,
> 
> Maxime seems to have a need for a bit more than what the current
> drm_mode_config_reste can do, so here's a bunch of ideas inspired by
> i915.
> 
> I think minimally what you need is a drm_atomic_state_helper_readout()
> functions, which instead of resetting, allocates all the obj->state
> pointers and fills them out. For that I think the simplest is to add
> atomic_readout_state functions to crtc, connector and plane (if you
> want to take over the firmware fb allocation too), which take as
> parameter the object, and return the read-out state. Important, they
> must _not_ touch anything persistent, otherwise the following stuff
> here doesn't work.
> 
> Next up is the challenge of bridges and encoders. If the goal is to
> properly shut down encoders/bridges, they also need their state. And
> on most hw, they need a mix of the crtc and connector state, so best
> to pass both of those (plus bridge state for bridges) to them. You can
> do that if we assume that connector_helper_funcs->atomic_readout_state
> sets the drm_connector_state->crtc pointer correctly. So the
> drm_atomic_state_helper_readout function would first loop through
> connectors and crtcs, and then loop through encoders and bridges to
> fill in the gaps. Last you probably want to go through planes.
> 
> Now even more fun hw will have trouble and might need to look up
> random other objects to set stuff, so we need a drm_atomic_state
> structure to tie all these things together. For reasons that will
> become obvious later on these read-out states should be stored in the
> old_ state pointers.
> 
> Finally we need the actual helper which takes that read-out state and
> smashes it into the real obj->state pointers, essentially a swap_state
> but in reverse (since we need to write the old_ state pointers into
> obj->state).
> 
> One thing i915 does, but I don't think is really needed: We read out
> the connector->crtc routing as a first step, and once we have that, we
> read out the connector/encoder/crtc steps. I think if you first read
> out (and hence allocate) crtrc states, and then connector, and then
> encoder/bridges that should work, and simplifies the flow a bit. So we
> need another drm_atomic_state_helper_reset_to_readout or whatever,
> which uses _readout and then does the reverse swap. Drivers call this
> instead of drm_mode_config_reset.
> 
> Now the real issue: How do you make sure this actually works? Testing
> with different fw configurations is usually impossible, you cant
> easily tell the firmware to boot with different modes. Or at least
> it's cumbersome since manual testing and lots of reboots. Waiting for
> bug reports and then fixing them, while probably breaking something
> else is a game of whack-a-mole.
> 
> So what i915 does is read out the hw state on every nonblocking
> modeset (the additional time spent doesn't matter), but _only_ for the
> objects touched in that modeset state. This is why you need to read
> out into old_ state pointers, since after a blocking modeset those are
> unused and available.

I have a feeling this old vs. new thing is still going to bite
someone. But atm don't really have any sane alternative ideas.

Hmm, maybe we could at least tag the atomic_state as "readout only"
for the duration of the actual readout and WARN/fail if anyone does
drm_atomic_get_new_foo_state() and for_each_new/oldnew...() on it?

> Next item is to add a  atomic_compare_state
> function to crtc/connector&plane and maybe bridges (i.e. all objects
> with state), which compares 2 state objects for equality. This needs
> to be a driver callback since each driver will only read out the state
> relevant from take-over from fw, not every possible feature, so
> there's lots you need to ignore. If any of these functions note a
> mismatch you splat with a warning and dump both the old and new states
> with the atomic_print driver hooks. I915 uses some #define so that
> these comparisons are one-liners (see PIPE_CONFIG_CHECK_X/I and so on,
> maybe we should have a few default ones with proper atomic naming, the
> names date back to the first somewhat-atomic modeset flow in i915).
> 
> So for validation we need a drm_atomic_state_helper_check which uses
> _readout, and then the compare functions plus debug printouts if it
> goes wrong. I'd wire that directly into the default
> drm_atomic_helper_commit function.
> 
> With these pieces you should have a state readout code that actually
> tends to work, and you can even test it (simply by doing a bunch of
> modesets). In i915 we have the _check code running unconditionally,
> blocking modesets are slow enough that it really doesn't matter.
> 
> One more thing on the implementation: Since this is all helpers all
> the hooks should probably be in the respective helper function tables.
> 
> Cheers, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel