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; ... -- Ville Syrjälä Intel
