[Intel-gfx] [PATCH v19 4/8] drm/i915: Refactor intel_can_enable_sagv

[Lisovskiy, Stanislav]

On Wed, Mar 18, 2020 at 02:50:55PM +0200, Ville Syrjälä wrote:
> On Wed, Mar 18, 2020 at 11:52:13AM +0000, Lisovskiy, Stanislav wrote:
> > >> @@ -5829,6 +6068,10 @@ skl_compute_wm(struct intel_atomic_state *state)
> > >>                        return ret;
> > >>        }
> > >>
> > >> +     ret = intel_compute_sagv_mask(state);
> > >> +     if (ret)
> > >> +             return ret;
> > 
> > > This seems too early. We haven't even computed the ddb yet.
> > 
> > 
> > I was thinking about our discussion last week and actually I think there are simply two ways how
> > 
> > to do it.
> > 
> > 
> > 1) What I do here is: calculate minimum amount required to fit SAGV wm levels into ddb and
> > 
> > based on that do the ddb allocation accordingly. I.e it is not to early because actually we have
> > 
> > already wm levels for sagv and non-sagv calculated - we already can check if it can fit into L0
> > 
> > and then act accordingly.
> > 
> > However one thing to consider here: as you said besides the minimal requirements for each plane
> > 
> > (with or without sagv) there is an extra space being allocated in proportion to plane data rate,
> > 
> > however here we are actually hitting the prioritization issue - i.e we need to decide whether
> > 
> > it is more important to have SAGV or to have more extra space allocated to different planes
> > 
> > proportionally to their needs.
> > 
> > So in this first approach we always first determine if we fit into minimum SAGV reqs, turn it
> > 
> > on if we do and then rest of extra space is allocated among the planes in proportion to data rate.
> > 
> > So that way we would be more often power efficient but but planes get less extra ddb space.
> > 
> > 
> > 2) In your approach we should calculate ddb first, allocate extra space proportionally to plane
> > 
> > data rate needs and only then check if all planes got enough space for L0 SAGV wm after that.
> > 
> > Then we actually don't even need skl_plane_wm_level accessor, because we first would be allocating
> > 
> > using normal wm levels + extra ddb and only then check if all planes fit into SAGV requirement -
> > 
> > because that extra space is not actually distributed evenly but in proportion to data rate of each
> > 
> > plane, which means that some planes might lack space for SAGV theoretically, because some might be
> > 
> > getting more or less depending on the data_rate/total_data_rate ratio.
> > 
> > 
> > My position is such that I'm really not like "my approach should always win" here, but more searching for
> > 
> > solution which is more correct from product point of view.
> > 
> > 
> > Also could be that it doesn't really matter which approach we do take now,, but matter more like
> > 
> > that how fast we deliver.  Because the actual outcome difference between two
> > 
> > might be minor, while time overhead for changing the approach could be major.
> 
> Pls fix your MUA. Really hard to read this.
> 
> -- 
> Ville Syrjälä
> Intel

I was thinking about our discussion last week and actually I think there are simply two ways how
to do it.

1) What I do here is: calculate minimum amount required to fit SAGV wm levels into ddb and
based on that do the ddb allocation accordingly. I.e it is not to early because actually we have
already wm levels for sagv and non-sagv calculated - we already can check if it can fit into L0
and then act accordingly.
However one thing to consider here: as you said besides the minimal requirements for each plane
(with or without sagv) there is an extra space being allocated in proportion to plane data rate,
however here we are actually hitting the prioritization issue - i.e we need to decide whether
it is more important to have SAGV or to have more extra space allocated to different planes
proportionally to their needs.
So in this first approach we always first determine if we fit into minimum SAGV reqs, turn it
on if we do and then rest of extra space is allocated among the planes in proportion to data rate.
So that way we would be more often power efficient but but planes get less extra ddb space.

2) In your approach we should calculate ddb first, allocate extra space proportionally to plane
data rate needs and only then check if all planes got enough space for L0 SAGV wm after that.
Then we actually don't even need skl_plane_wm_level accessor, because we first would be allocating
using normal wm levels + extra ddb and only then check if all planes fit into SAGV requirement -
because that extra space is not actually distributed evenly but in proportion to data rate of each
plane, which means that some planes might lack space for SAGV theoretically, because some might be
getting more or less depending on the data_rate/total_data_rate ratio.

My position is such that I'm really not like "my approach should always win" here, but more searching for
solution which is more correct from product point of view.

Also could be that it doesn't really matter which approach we do take now,, but matter more like
that how fast we deliver.  Because the actual outcome difference between two
might be minor, while time overhead for changing the approach could be major.