Re: [Intel-gfx] [PATCH 2/6] drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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