Re: [Intel-gfx] [PATCH v18 4/8] drm/i915: Introduce more *_state_changed indicators

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

On Tue, Feb 25, 2020 at 04:57:33PM +0200, Stanislav Lisovskiy wrote:
> The reasoning behind this is such that current dependencies
> in the code are rather implicit in a sense, we have to constantly
> check a bunch of different bits like state->modeset,
> state->active_pipe_changes, which sometimes can indicate counter
> intuitive changes.
> 
> By introducing more fine grained state change tracking we achieve
> better readability and dependency maintenance for the code.
> 
> For example it is no longer needed to evaluate active_pipe_changes
> to understand if there were changes for wm/ddb - lets just have
> a correspondent bit in a state, called ddb_state_changed.
> 
> active_pipe_changes just indicate whether there was some pipe added
> or removed. Then we evaluate if wm/ddb had been changed.
> Same for sagv/bw state. ddb changes may or may not affect if out
> bandwidth constraints have been changed.
> 
> v2: Add support for older Gens in order not to introduce regressions
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_atomic.c   |  2 ++
>  drivers/gpu/drm/i915/display/intel_bw.c       | 28 ++++++++++++++--
>  drivers/gpu/drm/i915/display/intel_display.c  | 16 ++++++----
>  .../drm/i915/display/intel_display_types.h    | 32 ++++++++++++-------
>  drivers/gpu/drm/i915/intel_pm.c               |  5 ++-
>  5 files changed, 62 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
> index d043057d2fa0..0db9c66d3c0f 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -525,6 +525,8 @@ void intel_atomic_state_clear(struct drm_atomic_state *s)
>  	state->dpll_set = state->modeset = false;
>  	state->global_state_changed = false;
>  	state->active_pipes = 0;
> +	state->ddb_state_changed = false;
> +	state->bw_state_changed = false;

Not really liking these.

After some pondering I was thinking along the lines of something simple
like this:

struct bw_state {
	u8 sagv_reject;
};

bw_check()
{
	for_each_crtc_in_state() {
		if (sagv_possible(crtc_state))
			new->sagv_reject &= ~BIT(pipe);
		else
			new->sagv_reject |= BIT(pipe);
	}

	calculate new->qgv_mask
}


>  }
>  
>  struct intel_crtc_state *
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> index bdad7476dc7b..d5be603b8b03 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.c
> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> @@ -424,9 +424,27 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
>  	struct intel_crtc *crtc;
>  	int i, ret;
>  
> -	/* FIXME earlier gens need some checks too */
> -	if (INTEL_GEN(dev_priv) < 11)
> +	/*
> +	 * For earlier Gens let's consider bandwidth changed if ddb requirements,
> +	 * has been changed.
> +	 */
> +	if (INTEL_GEN(dev_priv) < 11) {
> +		if (state->ddb_state_changed) {
> +			bw_state = intel_bw_get_state(state);
> +			if (IS_ERR(bw_state))
> +				return PTR_ERR(bw_state);
> +
> +			ret = intel_atomic_lock_global_state(&bw_state->base);
> +			if (ret)
> +				return ret;
> +
> +			DRM_DEBUG_KMS("Marking bw state changed for atomic state %p\n",
> +				      state);
> +
> +			state->bw_state_changed = true;
> +		}
>  		return 0;
> +	}
>  
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  					    new_crtc_state, i) {
> @@ -447,7 +465,7 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
>  		    old_active_planes == new_active_planes)
>  			continue;
>  
> -		bw_state  = intel_bw_get_state(state);
> +		bw_state = intel_bw_get_state(state);
>  		if (IS_ERR(bw_state))
>  			return PTR_ERR(bw_state);
>  
> @@ -468,6 +486,10 @@ int intel_bw_atomic_check(struct intel_atomic_state *state)
>  	if (ret)
>  		return ret;
>  
> +	DRM_DEBUG_KMS("Marking bw state changed for atomic state %p\n", state);
> +
> +	state->bw_state_changed = true;
> +
>  	data_rate = intel_bw_data_rate(dev_priv, bw_state);
>  	num_active_planes = intel_bw_num_active_planes(dev_priv, bw_state);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 3031e64ee518..137fb645097a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -15540,8 +15540,10 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  		 * SKL workaround: bspec recommends we disable the SAGV when we
>  		 * have more then one pipe enabled
>  		 */
> -		if (!intel_can_enable_sagv(state))
> -			intel_disable_sagv(dev_priv);
> +		if (state->bw_state_changed) {
> +			if (!intel_can_enable_sagv(state))
> +				intel_disable_sagv(dev_priv);
> +		}
>  
>  		intel_modeset_verify_disabled(dev_priv, state);
>  	}
> @@ -15565,7 +15567,7 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  		intel_encoders_update_prepare(state);
>  
>  	/* Enable all new slices, we might need */
> -	if (state->modeset)
> +	if (state->ddb_state_changed)
>  		icl_dbuf_slice_pre_update(state);
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
> @@ -15622,7 +15624,7 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  	}
>  
>  	/* Disable all slices, we don't need */
> -	if (state->modeset)
> +	if (state->ddb_state_changed)
>  		icl_dbuf_slice_post_update(state);
>  
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> @@ -15641,8 +15643,10 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  	if (state->modeset)
>  		intel_verify_planes(state);
>  
> -	if (state->modeset && intel_can_enable_sagv(state))
> -		intel_enable_sagv(dev_priv);
> +	if (state->bw_state_changed) {
> +		if (intel_can_enable_sagv(state)
> +			intel_enable_sagv(dev_priv);
> +	}
>  
>  	drm_atomic_helper_commit_hw_done(&state->base);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 0d8a64305464..12b47ba3c68d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -471,16 +471,6 @@ struct intel_atomic_state {
>  
>  	bool dpll_set, modeset;
>  
> -	/*
> -	 * Does this transaction change the pipes that are active?  This mask
> -	 * tracks which CRTC's have changed their active state at the end of
> -	 * the transaction (not counting the temporary disable during modesets).
> -	 * This mask should only be non-zero when intel_state->modeset is true,
> -	 * but the converse is not necessarily true; simply changing a mode may
> -	 * not flip the final active status of any CRTC's
> -	 */
> -	u8 active_pipe_changes;
> -
>  	u8 active_pipes;
>  
>  	struct intel_shared_dpll_state shared_dpll[I915_NUM_PLLS];
> @@ -494,10 +484,30 @@ struct intel_atomic_state {
>  	bool rps_interactive;
>  
>  	/*
> -	 * active_pipes
> +	 * active pipes
>  	 */
>  	bool global_state_changed;
>  
> +	/*
> +	 * Does this transaction change the pipes that are active?  This mask
> +	 * tracks which CRTC's have changed their active state at the end of
> +	 * the transaction (not counting the temporary disable during modesets).
> +	 * This mask should only be non-zero when intel_state->modeset is true,
> +	 * but the converse is not necessarily true; simply changing a mode may
> +	 * not flip the final active status of any CRTC's
> +	 */
> +	u8 active_pipe_changes;
> +
> +	/*
> +	 * More granular change indicator for ddb changes
> +	 */
> +	bool ddb_state_changed;
> +
> +	/*
> +	 * More granular change indicator for bandwidth state changes
> +	 */
> +	bool bw_state_changed;
> +
>  	/* Number of enabled DBuf slices */
>  	u8 enabled_dbuf_slices_mask;
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 409b91c17a7f..ac4b317ea1bf 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3894,7 +3894,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
>  	 * that changes the active CRTC list or do modeset would need to
>  	 * grab _all_ crtc locks, including the one we currently hold.
>  	 */
> -	if (!intel_state->active_pipe_changes && !intel_state->modeset) {
> +	if (!intel_state->ddb_state_changed) {
>  		/*
>  		 * alloc may be cleared by clear_intel_crtc_state,
>  		 * copy from old state to be sure
> @@ -5787,6 +5787,9 @@ static int skl_wm_add_affected_planes(struct intel_atomic_state *state,
>  			return PTR_ERR(plane_state);
>  
>  		new_crtc_state->update_planes |= BIT(plane_id);
> +
> +		DRM_DEBUG_KMS("Marking ddb state changed for atomic state %p\n", state);
> +		state->ddb_state_changed = true;
>  	}
>  
>  	return 0;
> -- 
> 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