On Thu, Apr 30, 2020 at 12:21:04PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 23, 2020 at 10:58:56AM +0300, Stanislav Lisovskiy wrote:
> > We need to calculate SAGV mask also in a non-modeset
> > commit, however currently active_pipes are only calculated
> > for modesets in global atomic state, thus now we will be
> > tracking those also in bw_state in order to be able to
> > properly access global data.
> >
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_bw.h | 3 +++
> > drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++-----
> > 2 files changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> > index d6df91058223..898b4a85ccab 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bw.h
> > +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> > @@ -26,6 +26,9 @@ struct intel_bw_state {
> >
> > unsigned int data_rate[I915_MAX_PIPES];
> > u8 num_active_planes[I915_MAX_PIPES];
> > +
> > + /* bitmask of active pipes */
> > + u8 active_pipes;
> > };
> >
> > #define to_intel_bw_state(x) container_of((x), struct intel_bw_state, base)
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 7e15cf3368ad..f7249bca3f6f 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3874,6 +3874,7 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state)
> > struct intel_bw_state *new_bw_state = NULL;
> > const struct intel_bw_state *old_bw_state = NULL;
> > int i;
> > + bool active_pipes_calculated = false;
> >
> > for_each_new_intel_crtc_in_state(state, crtc,
> > new_crtc_state, i) {
> > @@ -3883,6 +3884,12 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state)
> >
> > old_bw_state = intel_atomic_get_old_bw_state(state);
> >
> > + if (!active_pipes_calculated) {
> > + state->active_pipes = new_bw_state->active_pipes =
>
> I don't think we should touch state->active_pipes here.
Well, that was my question actually here as well. I understand that changing
state->active_pipes here feels like some unneeded side effect, however having
state->active_pipes and bw_state->active_pipes going out of sync doesn't sound
very attractive to me either. That is why I don't like this idea of duplication
at all - having constant need to sync those state, bw_state, cdclk_state, because
they all might have different active_pipes now.
>
> > + intel_calc_active_pipes(state, old_bw_state->active_pipes);
> > + active_pipes_calculated = true;
> > + }
>
> I'd do this after the loop so we don't need this extra boolean. As far
> as the active_pipes check in intel_crtc_can_enable_sagv(), I think we
> can pull it out into intel_compute_sagv_mask() so that we do the check
> after computing the mask. And of course change it to use
> bw_state->active_pipes instead.
intel_crtc_can_enable_sagv is called per crtc - so can't just pull it out,
will have to have to cycles then - one will compute bw_state->active_pipes,
and another pipe_sagv_mask.
>
> We're also going to need to lock_global_state() if bw_state->active_pipes
> mask changes.
Ohh.. right.
Stan
>
> > +
> > if (intel_crtc_can_enable_sagv(new_crtc_state))
> > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe);
> > else
> > @@ -5911,11 +5918,9 @@ skl_compute_wm(struct intel_atomic_state *state)
> > if (ret)
> > return ret;
> >
> > - if (state->modeset) {
> > - ret = intel_compute_sagv_mask(state);
> > - if (ret)
> > - return ret;
> > - }
> > + ret = intel_compute_sagv_mask(state);
> > + if (ret)
> > + return ret;
>
> We also need to remove the state->modeset checks around
> sagv_{pre,post}_update().
>
> >
> > /*
> > * skl_compute_ddb() will have adjusted the final watermarks
> > --
> > 2.24.1.485.gad05a3d8e5
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
