Re: [Intel-gfx] [PATCH] drm/i915/adl: Initialize all GV points as restricted in bw_state

[Jani Nikula <jani.nikula@linux.intel.com>]

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);
>  	}

-- 
Jani Nikula, Intel