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 -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
