| Message ID | 20231024125256.11954-1-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/adl: Initialize all GV points as restricted in bw_state | expand |
On Tue, 24 Oct 2023, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote: > In some customer cases, machine can start up with all > GV points restricted. However we don't ever read those > from hw and initial driver qgv_points_mask is initialized > as 0, which would make driver think that all points are unrestricted, > so we never update them with proper value, unless > some demanding scenario is requested or we have to toggle SAGV > and we have to restrict some of those. > Lets fix that by initializing all points as restricted, > then on first modeset, that will make sure driver will naturally > calculate, which of those need to be relaxed and do correspondent update. > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_bw.c | 7 ++++--- > drivers/gpu/drm/i915/display/intel_bw.h | 1 + > drivers/gpu/drm/i915/display/intel_modeset_setup.c | 13 +++++++++++++ > 3 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c > index bef96db62c80..fbfa01f38db8 100644 > --- a/drivers/gpu/drm/i915/display/intel_bw.c > +++ b/drivers/gpu/drm/i915/display/intel_bw.c > @@ -119,7 +119,7 @@ static int adls_pcode_read_psf_gv_point_info(struct drm_i915_private *dev_priv, > return 0; > } > > -static u16 icl_qgv_points_mask(struct drm_i915_private *i915) > +u16 icl_qgv_points_mask(struct drm_i915_private *i915) > { > unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points; > unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points; > @@ -1277,9 +1277,10 @@ int intel_bw_atomic_check(struct intel_atomic_state *state) > > /* > * If none of our inputs (data rates, number of active > - * planes, SAGV yes/no) changed then nothing to do here. > + * planes, SAGV yes/no) changed then nothing to do here, > + * except if mask turns out to be in wrong state initially. > */ > - if (!changed) > + if (!changed && (new_bw_state->qgv_points_mask != icl_qgv_points_mask(i915))) > return 0; > > ret = intel_bw_check_qgv_points(i915, old_bw_state, new_bw_state); > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h > index 59cb4fc5db76..0a706ec79ce3 100644 > --- a/drivers/gpu/drm/i915/display/intel_bw.h > +++ b/drivers/gpu/drm/i915/display/intel_bw.h > @@ -70,6 +70,7 @@ void intel_bw_crtc_update(struct intel_bw_state *bw_state, > const struct intel_crtc_state *crtc_state); > int icl_pcode_restrict_qgv_points(struct drm_i915_private *dev_priv, > u32 points_mask); > +u16 icl_qgv_points_mask(struct drm_i915_private *i915); > int intel_bw_calc_min_cdclk(struct intel_atomic_state *state, > bool *need_cdclk_calc); > int intel_bw_min_cdclk(struct drm_i915_private *i915, > diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c b/drivers/gpu/drm/i915/display/intel_modeset_setup.c > index b8f43efb0ab5..230090c6e994 100644 > --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c > +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c > @@ -871,6 +871,19 @@ static void intel_modeset_readout_hw_state(struct drm_i915_private *i915) > intel_pmdemand_update_port_clock(i915, pmdemand_state, pipe, > crtc_state->port_clock); > > + /* > + * In some customer cases, machine can start up with all > + * GV points restricted. However we don't ever read those > + * from hw and qgv_points_mask initialized as 0, would > + * make driver think that all points are unrestricted, > + * so we never update them with proper value, unless > + * some demanding scenario is requested and we have to > + * restrict some of those. Lets fix that by initializing > + * all points as restricted, then on first modeset, driver > + * will naturally calculate, which of those need to be > + * relaxed and do correspondent update. > + */ > + bw_state->qgv_points_mask = icl_qgv_points_mask(i915); Sad trombone for having to expose highly specific functions and stuff from intel_bw.c. Can't the below call handle it? BR, Jani. > intel_bw_crtc_update(bw_state, crtc_state); > }
On Tue, Oct 24, 2023 at 03:52:56PM +0300, Stanislav Lisovskiy wrote: > In some customer cases, machine can start up with all > GV points restricted. However we don't ever read those > from hw and initial driver qgv_points_mask is initialized > as 0, which would make driver think that all points are unrestricted, > so we never update them with proper value, unless > some demanding scenario is requested or we have to toggle SAGV > and we have to restrict some of those. > Lets fix that by initializing all points as restricted, > then on first modeset, that will make sure driver will naturally > calculate, which of those need to be relaxed and do correspondent update. > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_bw.c | 7 ++++--- > drivers/gpu/drm/i915/display/intel_bw.h | 1 + > drivers/gpu/drm/i915/display/intel_modeset_setup.c | 13 +++++++++++++ > 3 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c > index bef96db62c80..fbfa01f38db8 100644 > --- a/drivers/gpu/drm/i915/display/intel_bw.c > +++ b/drivers/gpu/drm/i915/display/intel_bw.c > @@ -119,7 +119,7 @@ static int adls_pcode_read_psf_gv_point_info(struct drm_i915_private *dev_priv, > return 0; > } > > -static u16 icl_qgv_points_mask(struct drm_i915_private *i915) > +u16 icl_qgv_points_mask(struct drm_i915_private *i915) > { > unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points; > unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points; > @@ -1277,9 +1277,10 @@ int intel_bw_atomic_check(struct intel_atomic_state *state) > > /* > * If none of our inputs (data rates, number of active > - * planes, SAGV yes/no) changed then nothing to do here. > + * planes, SAGV yes/no) changed then nothing to do here, > + * except if mask turns out to be in wrong state initially. > */ > - if (!changed) > + if (!changed && (new_bw_state->qgv_points_mask != icl_qgv_points_mask(i915))) There doesn't seem to be any guarantee that the bw state is actually present here. So this could oops. But I'm thinking a better fix might be to do what we on older platforms and just force SAGV off at the start. That way we actually know what state the hardware will be in. > return 0; > > ret = intel_bw_check_qgv_points(i915, old_bw_state, new_bw_state); > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h > index 59cb4fc5db76..0a706ec79ce3 100644 > --- a/drivers/gpu/drm/i915/display/intel_bw.h > +++ b/drivers/gpu/drm/i915/display/intel_bw.h > @@ -70,6 +70,7 @@ void intel_bw_crtc_update(struct intel_bw_state *bw_state, > const struct intel_crtc_state *crtc_state); > int icl_pcode_restrict_qgv_points(struct drm_i915_private *dev_priv, > u32 points_mask); > +u16 icl_qgv_points_mask(struct drm_i915_private *i915); > int intel_bw_calc_min_cdclk(struct intel_atomic_state *state, > bool *need_cdclk_calc); > int intel_bw_min_cdclk(struct drm_i915_private *i915, > diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c b/drivers/gpu/drm/i915/display/intel_modeset_setup.c > index b8f43efb0ab5..230090c6e994 100644 > --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c > +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c > @@ -871,6 +871,19 @@ static void intel_modeset_readout_hw_state(struct drm_i915_private *i915) > intel_pmdemand_update_port_clock(i915, pmdemand_state, pipe, > crtc_state->port_clock); > > + /* > + * In some customer cases, machine can start up with all > + * GV points restricted. However we don't ever read those > + * from hw and qgv_points_mask initialized as 0, would > + * make driver think that all points are unrestricted, > + * so we never update them with proper value, unless > + * some demanding scenario is requested and we have to > + * restrict some of those. Lets fix that by initializing > + * all points as restricted, then on first modeset, driver > + * will naturally calculate, which of those need to be > + * relaxed and do correspondent update. > + */ > + bw_state->qgv_points_mask = icl_qgv_points_mask(i915); > intel_bw_crtc_update(bw_state, crtc_state); > } > > -- > 2.37.3
On Tue, Oct 24, 2023 at 05:01:22PM +0300, Ville Syrjälä wrote: > On Tue, Oct 24, 2023 at 03:52:56PM +0300, Stanislav Lisovskiy wrote: > > In some customer cases, machine can start up with all > > GV points restricted. However we don't ever read those > > from hw and initial driver qgv_points_mask is initialized > > as 0, which would make driver think that all points are unrestricted, > > so we never update them with proper value, unless > > some demanding scenario is requested or we have to toggle SAGV > > and we have to restrict some of those. > > Lets fix that by initializing all points as restricted, > > then on first modeset, that will make sure driver will naturally > > calculate, which of those need to be relaxed and do correspondent update. > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_bw.c | 7 ++++--- > > drivers/gpu/drm/i915/display/intel_bw.h | 1 + > > drivers/gpu/drm/i915/display/intel_modeset_setup.c | 13 +++++++++++++ > > 3 files changed, 18 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c > > index bef96db62c80..fbfa01f38db8 100644 > > --- a/drivers/gpu/drm/i915/display/intel_bw.c > > +++ b/drivers/gpu/drm/i915/display/intel_bw.c > > @@ -119,7 +119,7 @@ static int adls_pcode_read_psf_gv_point_info(struct drm_i915_private *dev_priv, > > return 0; > > } > > > > -static u16 icl_qgv_points_mask(struct drm_i915_private *i915) > > +u16 icl_qgv_points_mask(struct drm_i915_private *i915) > > { > > unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points; > > unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points; > > @@ -1277,9 +1277,10 @@ int intel_bw_atomic_check(struct intel_atomic_state *state) > > > > /* > > * If none of our inputs (data rates, number of active > > - * planes, SAGV yes/no) changed then nothing to do here. > > + * planes, SAGV yes/no) changed then nothing to do here, > > + * except if mask turns out to be in wrong state initially. > > */ > > - if (!changed) > > + if (!changed && (new_bw_state->qgv_points_mask != icl_qgv_points_mask(i915))) > > There doesn't seem to be any guarantee that the bw state is > actually present here. So this could oops. > > But I'm thinking a better fix might be to do what we on > older platforms and just force SAGV off at the start. That > way we actually know what state the hardware will be in. If you mean that by forcing SAGV off, we will also update QGV point mask, leaving only the highest enabled, then yeah that could be better solution. Thing is that, as we can't read QGV point mask directly from HW, it is better to always update it initially, to avoid that kind of issues. Stan > > > return 0; > > > > ret = intel_bw_check_qgv_points(i915, old_bw_state, new_bw_state); > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h > > index 59cb4fc5db76..0a706ec79ce3 100644 > > --- a/drivers/gpu/drm/i915/display/intel_bw.h > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h > > @@ -70,6 +70,7 @@ void intel_bw_crtc_update(struct intel_bw_state *bw_state, > > const struct intel_crtc_state *crtc_state); > > int icl_pcode_restrict_qgv_points(struct drm_i915_private *dev_priv, > > u32 points_mask); > > +u16 icl_qgv_points_mask(struct drm_i915_private *i915); > > int intel_bw_calc_min_cdclk(struct intel_atomic_state *state, > > bool *need_cdclk_calc); > > int intel_bw_min_cdclk(struct drm_i915_private *i915, > > diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c b/drivers/gpu/drm/i915/display/intel_modeset_setup.c > > index b8f43efb0ab5..230090c6e994 100644 > > --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c > > +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c > > @@ -871,6 +871,19 @@ static void intel_modeset_readout_hw_state(struct drm_i915_private *i915) > > intel_pmdemand_update_port_clock(i915, pmdemand_state, pipe, > > crtc_state->port_clock); > > > > + /* > > + * In some customer cases, machine can start up with all > > + * GV points restricted. However we don't ever read those > > + * from hw and qgv_points_mask initialized as 0, would > > + * make driver think that all points are unrestricted, > > + * so we never update them with proper value, unless > > + * some demanding scenario is requested and we have to > > + * restrict some of those. Lets fix that by initializing > > + * all points as restricted, then on first modeset, driver > > + * will naturally calculate, which of those need to be > > + * relaxed and do correspondent update. > > + */ > > + bw_state->qgv_points_mask = icl_qgv_points_mask(i915); > > intel_bw_crtc_update(bw_state, crtc_state); > > } > > > > -- > > 2.37.3 > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c index bef96db62c80..fbfa01f38db8 100644 --- a/drivers/gpu/drm/i915/display/intel_bw.c +++ b/drivers/gpu/drm/i915/display/intel_bw.c @@ -119,7 +119,7 @@ static int adls_pcode_read_psf_gv_point_info(struct drm_i915_private *dev_priv, return 0; } -static u16 icl_qgv_points_mask(struct drm_i915_private *i915) +u16 icl_qgv_points_mask(struct drm_i915_private *i915) { unsigned int num_psf_gv_points = i915->display.bw.max[0].num_psf_gv_points; unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points; @@ -1277,9 +1277,10 @@ int intel_bw_atomic_check(struct intel_atomic_state *state) /* * If none of our inputs (data rates, number of active - * planes, SAGV yes/no) changed then nothing to do here. + * planes, SAGV yes/no) changed then nothing to do here, + * except if mask turns out to be in wrong state initially. */ - if (!changed) + if (!changed && (new_bw_state->qgv_points_mask != icl_qgv_points_mask(i915))) return 0; ret = intel_bw_check_qgv_points(i915, old_bw_state, new_bw_state); diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h index 59cb4fc5db76..0a706ec79ce3 100644 --- a/drivers/gpu/drm/i915/display/intel_bw.h +++ b/drivers/gpu/drm/i915/display/intel_bw.h @@ -70,6 +70,7 @@ void intel_bw_crtc_update(struct intel_bw_state *bw_state, const struct intel_crtc_state *crtc_state); int icl_pcode_restrict_qgv_points(struct drm_i915_private *dev_priv, u32 points_mask); +u16 icl_qgv_points_mask(struct drm_i915_private *i915); int intel_bw_calc_min_cdclk(struct intel_atomic_state *state, bool *need_cdclk_calc); int intel_bw_min_cdclk(struct drm_i915_private *i915, diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c b/drivers/gpu/drm/i915/display/intel_modeset_setup.c index b8f43efb0ab5..230090c6e994 100644 --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c @@ -871,6 +871,19 @@ static void intel_modeset_readout_hw_state(struct drm_i915_private *i915) intel_pmdemand_update_port_clock(i915, pmdemand_state, pipe, crtc_state->port_clock); + /* + * In some customer cases, machine can start up with all + * GV points restricted. However we don't ever read those + * from hw and qgv_points_mask initialized as 0, would + * make driver think that all points are unrestricted, + * so we never update them with proper value, unless + * some demanding scenario is requested and we have to + * restrict some of those. Lets fix that by initializing + * all points as restricted, then on first modeset, driver + * will naturally calculate, which of those need to be + * relaxed and do correspondent update. + */ + bw_state->qgv_points_mask = icl_qgv_points_mask(i915); intel_bw_crtc_update(bw_state, crtc_state); }
In some customer cases, machine can start up with all GV points restricted. However we don't ever read those from hw and initial driver qgv_points_mask is initialized as 0, which would make driver think that all points are unrestricted, so we never update them with proper value, unless some demanding scenario is requested or we have to toggle SAGV and we have to restrict some of those. Lets fix that by initializing all points as restricted, then on first modeset, that will make sure driver will naturally calculate, which of those need to be relaxed and do correspondent update. Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- drivers/gpu/drm/i915/display/intel_bw.c | 7 ++++--- drivers/gpu/drm/i915/display/intel_bw.h | 1 + drivers/gpu/drm/i915/display/intel_modeset_setup.c | 13 +++++++++++++ 3 files changed, 18 insertions(+), 3 deletions(-)