On Fri, May 15, 2020 at 04:51:53PM +0300, Ville Syrjälä wrote:
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?
Oh, and I also had some fun with the readout overwriting old_crtc_state->commit when I playing around with vblank workers. I'm not even sure why that doesn't blow up currently as well.
I think we should try to remove that confusing new_crtc_state->commit vs. old_crtc_state->commit switcheroo. But I have a feeling that is going to require quite a few changes since I guess it's the old_crtc_state that currently gets plumbed into various places.
Another related annoyance is the old_foo_state->state vs. new_foo_state->state which routinely trips up people with null ptr derefs.
I guess trying to type up some cocci to plumb drm_atomic_state everywhere might be a start...