Hi,
On 04/09/2020 05:29, Laurent Pinchart wrote:
Laurent mentioned that atomic_check should not change state. Note that cdns_mhdp_validate_mode_params also changes state, as it calculates tu_size, vs and line_thresh.
.atomic_check() isn't allowed to change any global state, which means both hardware state and data in cdns_mhdp_device. The drm_bridge_state (and thus the cdns_mhdp_bridge_state) can be modified as it stores the state for the atomic commit being checked.
There seems to be issues with mode changes, but I think the first step would be to clarify the related code a bit. cdns_mhdp_validate_mode_params() is misnamed, I think it should be renamed to calculate_tu or something like that.
cdns_mhdp_bandwidth_ok() should take display_fmt or bpp as a parameter, as currently it digs that up from the current state.
Probably cdns_mhdp_validate_mode_params() would be better if it doesn't write the result to the state, but returns the values. That way it could also be used to verify if suitable settings can be found, without changing the state.
This use case is actually a very good example of proper usage of the atomic state :-) .atomic_check() has to perform computations to verify the atomic commit, and storing the results in the commit's state prevents duplicating the same calculation at .atomic_commit() time.
Yes, you're right.
But it's still not good, as cdns_mhdp_validate_mode_params uses link details to do the calculations, but we do link training only later and thus the calculations are invalid.
cdns_mhdp_validate_mode_params is also called from the HPD interrupt, and there it changes the current bridge state. link_mutex is being held in every place where cdns_mhdp_validate_mode_params is called, so I guess it's fine.
Tomi