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... -- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
