| Message ID | 20240405113533.338553-7-vinod.govindapillai@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | QGV/SAGV related fixes | expand |
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. > > 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
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. > > Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com> Hmmm.. the whole point of that series is actually to put HW/SW in sync, before we actually are able to calculate the real requirements. When we initially for QGV/PSF GV to the highest point(thus disabling SAGV), we exactly want to make sure that HW/SW are in sync(which wasn't the case before that). Then later when the real plane bw requirements are calculated, we can possibly relax the QGV point requirements, enabling more points. I don't see why we need to force the recalculation here. Or am I missing something? Stan > --- > 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 >
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.Ithinksuchascenariohappensonlyonceaftertheboot.SothatsthereasonIaddedthisflagtoforcecalculateQGV. 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 >
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 > > >
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];
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. 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(-)