Re: [PATCH v10 6/6] drm/i915/display: force qgv check after the hw state readout

["Govindapillai, Vinod" <vinod.govindapillai@intel.com>]

On Tue, 2024-04-23 at 10:40 +0300, Govindapillai, Vinod wrote:
> On Fri, 2024-04-19 at 20:19 +0300, Ville Syrjälä wrote:
> > On Fri, Apr 05, 2024 at 02:35:33PM +0300, Vinod Govindapillai wrote:
> > > The current intel_bw_atomic_check do not check the possbility
> > > of a sagv configuration change after the hw state readout.
> > > Hence cannot update the sagv configuration until some other
> > > relevant changes like data rates, number of planes etc. happen.
> > > Introduce a flag to force qgv check in such cases.
> > 
> > The correct fix would be to make sure the readout actually
> > works, and thus the software state reflects the state of
> > the hardare accurately.
> 
> Okay! Does that mean, we need to update the qgv points and update to pcode right after the HW
> readout?
> 
> Right now, on bw_init we force disable sagv, so that we have a known sagv state. Then on
>         intel_modeset_readout_hw_state() we update the bw_state parameters. SAGV is still
> disabled. So on
> nextmodesetifthebw_stateparametersaresameastheoldm,thenwewillnotbecalculatingtheQGVpoints.Ithinksu
> chascenariohappensonlyonceaftertheboot.SothatsthereasonIaddedthisflagtoforcecalculateQGV.

Resending the previous reply as somehow the format got messed up!

Okay! Does that mean, we need to update the qgv points and update to pcode right after the HW
readout?

Right now, on bw_init we force disable sagv, so that we have a known sagv state. Then on
intel_modeset_readout_hw_state we update the bw_state parameters. SAGV is still disabled. So on
next modeset if the bw_state parameters are same as the old, then we will not be calculating the QGV
points. I think such a scenario happens only once after the boot. So thats the reason I added this
flag to force calculate QGV.

BR
Vinod

> 
> 
> BR
> Vinod
> 
> > 
> > > 
> > > Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_bw.c | 8 ++++++--
> > >  drivers/gpu/drm/i915/display/intel_bw.h | 6 ++++++
> > >  2 files changed, 12 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> > > index 6fb228a1a28f..1b190be745a0 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > > @@ -755,6 +755,7 @@ void intel_bw_crtc_update(struct intel_bw_state *bw_state,
> > >                 intel_bw_crtc_data_rate(crtc_state);
> > >         bw_state->num_active_planes[crtc->pipe] =
> > >                 intel_bw_crtc_num_active_planes(crtc_state);
> > > +       bw_state->force_check_qgv = true;
> > >  
> > >         drm_dbg_kms(&i915->drm, "pipe %c data rate %u num active planes %u\n",
> > >                     pipe_name(crtc->pipe),
> > > @@ -1339,8 +1340,9 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
> > >         new_bw_state = intel_atomic_get_new_bw_state(state);
> > >  
> > >         if (new_bw_state &&
> > > -           intel_can_enable_sagv(i915, old_bw_state) !=
> > > -           intel_can_enable_sagv(i915, new_bw_state))
> > > +           (intel_can_enable_sagv(i915, old_bw_state) !=
> > > +            intel_can_enable_sagv(i915, new_bw_state) ||
> > > +            new_bw_state->force_check_qgv))
> > >                 changed = true;
> > >  
> > >         /*
> > > @@ -1354,6 +1356,8 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
> > >         if (ret)
> > >                 return ret;
> > >  
> > > +       new_bw_state->force_check_qgv = false;
> > > +
> > >         return 0;
> > >  }
> > >  
> > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> > > index fa1e924ec961..161813cca473 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_bw.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> > > @@ -47,6 +47,12 @@ struct intel_bw_state {
> > >          */
> > >         u16 qgv_points_mask;
> > >  
> > > +       /*
> > > +        * Flag to force the QGV comparison in atomic check right after the
> > > +        * hw state readout
> > > +        */
> > > +       bool force_check_qgv;
> > > +
> > >         int min_cdclk[I915_MAX_PIPES];
> > >         unsigned int data_rate[I915_MAX_PIPES];
> > >         u8 num_active_planes[I915_MAX_PIPES];
> > > -- 
> > > 2.34.1
> > 
>