Hello Ville and Thomas, On 9/12/22 16:22, Thomas Zimmermann wrote: [...] >>>>>>> + >>>>>>> +/** >>>>>>> + * drm_plane_helper_atomic_check() - Helper to check primary planes states >>>>>>> + * @plane: plane to check >>>>>>> + * @new_state: plane state to check >>>>>> >>>>>> That is not a plane state. Also should s/new_// since it's just Right. What's the proper name? hardware state, modeset state, atomic state ? >>>>>> the overall atomic state thing rather than some new or old state. >>>>> >>>>> Using only 'state' is non-intuitive and has lead to bugs where sub-state >>>>> was retrieved from the wrong state information. So we've been using >>>>> 'new_state' and 'old_state' explicitly in several places now. >>>> >>>> There is no old or new drm_atomic_state. It contains both. >>> >>> I (vaguely) remember a bug where a driver tried >>> drm_atomic_get_new_plane_state() with the (old) state that's passed to >>> atomic_update. It didn't return the expected results and modesetting >>> gave slightly wrong results. >> >> As there is no wrong drm_atomic_state to pass I don't think it could >> have been the case. >> >>> So we began to be more precise about new >>> and old. And whatever is stored in 'plane->state' is then just 'the state'. >> >> There were certainly a lot of confusion before the explicit new/old >> state stuff was added whether foo->state/etc. was the old or the >> new state. And labeling things as explicitly old vs. new when passing >> in individual object states certainly makes sense. But that doesn't >> really have anything to do with mislabeling the overall drm_atomic_state. >> >>> >>> I understand that the semantics of atomic_check are different from >>> atomic_update, but it still doesn't hurt to talk of new_state IMHO. >> >> IMO it's just confusing. Makes the reader think there is somehow >> different drm_atomic_states for old vs. new states when there isn't. >> I also wouldn't call it new_state for .atomic_update() either. >> >> In both cases you have the old and new states in there and how >> exactly they get used in the hooks is more of an implementation >> detail. The only rules you would have to follow is that at the >> end of .atomic_update() the hardware state matches the new state, >> and .atomic_check() makes sure the transition from the old to the >> new state is possible. > > From what I understand: > > In atomic_check(), plane->state is the current state and the state > argument is the state to be validated. Calling > drm_atomic_get_new_plane_state() will return the plane's new state. > That's my understanding as well. > If you call drm_atomic_get_old_plane_state() from atomic_check(), what > will it return? > > In atomic_update() plane->state is the state to be committed and the > state argument is the old state before the start of the atomic commit. > And calling drm_atomic_get_new_plane_state() will *not* the return the > plane's new state (i.e., the one in plane->state) IIRC. (As I mentioned, > there was a related bug in one of the drivers.) So we began to call this > 'old_state'. > > My point is: the state passed to the check and commit functions are > different things, even though they appear to be the same. > Agree. Maybe instead of new and old `current_state` and `commit_state` would be more clear ? >> >> I've proposed renaming drm_atomic_state to eg. drm_atomic_transaction >> a few times before but no one took the bait so far... >> > > If you really don't like new_state, then let's call it state_tx. > I would prefer if for this patch we call it either `new_state` or just `state` to be consistent with the other helpers. And then we can as a follow-up change the naming used by all the helpers. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
