On Tue, Feb 15, 2022 at 06:58:35PM +0200, Ville Syrjälä wrote:
On Tue, Feb 15, 2022 at 06:52:51PM +0200, Ville Syrjälä wrote:
On Tue, Feb 15, 2022 at 06:33:42PM +0200, Lisovskiy, Stanislav wrote:
On Tue, Feb 15, 2022 at 01:26:50PM +0200, Ville Syrjälä wrote:
On Tue, Feb 15, 2022 at 01:02:48PM +0200, Lisovskiy, Stanislav wrote:
Anyway my point here is that, we probably shouldn't use new_bw_state as a way to check that plane allocations had changed. Thats just confusing.
We are not checking if plane allocations have changed. We are trying to determine if anything in the bw_state has changed. If we have said state already then something in it may have changed and we have to recalculate anything that may depend on those changed things, namely pipe_sagv_reject->qgv_point_mask.
I think it is just not very intuitive that we use the fact whether we can get new_bw_state or not, as a way to check if something had changed. Would be nice to put it in somekind of a wrapper like "has_new_bw_state" or "bw_state_changed". Because for anyone not quite familiar with that state paradigm we use, that would look pretty confusing that first we get new_bw_state using intel_atomic_get_new_bw_state, then immediately override it with intel_atomic_get_bw_state. And whether we can get new_bw_state or not is just acting like a check, that we don't have anything changed in bw_state.
I think the only thing we'd achieve is something like this:
new_bw_state = NULL; if (has_new_bw_state()) new_bw_state = get_new_bw_state(); ... if (!new_bw_state) return 0;
instead of just
new_bw_state = get_new_bw_state(); ... if (!new_bw_state) return 0;
I don't know why that would be an improvement.
Though I suppose a comment might be in order pointing the reader towards intel_compute_sagv_mask().
Although, I guess one idea would be to extract that data_rate loop thing into a separate function and then we'd just end up with something along the lines of:
ret = intel_bw_check_data_rate(state); if (ret) return ret;
new_bw_state = intel_atomic_get_new_bw_state(state); if (!new_bw_state) return 0;
...