drm state readout helpers

* drm state readout helpers
@ 2020-05-15 13:36 Daniel Vetter
  2020-05-15 13:51 ` Ville Syrjälä
  0 siblings, 1 reply; 3+ messages in thread
From: Daniel Vetter @ 2020-05-15 13:36 UTC (permalink / raw)
  To: Maxime Ripard, Syrjala, Ville; +Cc: dri-devel

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. 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread