Re: [Intel-gfx] [PATCH v26 3/9] drm/i915: Track active_pipes in bw_state

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@intel.com>
> ---
>  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.

> +				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.

We're also going to need to lock_global_state() if bw_state->active_pipes
mask changes.

> +
>  		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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx