Re: [Intel-gfx] [PATCH v8 2/4] drm/i915: Move dbuf slice update to proper place

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

On Fri, Dec 13, 2019 at 03:02:26PM +0200, Stanislav Lisovskiy wrote:
> Current DBuf slices update wasn't done in proper
> plane, especially its "post" part, which should
> disable those only once vblank had passed and
> all other changes are committed.
> 
> v2: Fix to use dev_priv and intel_atomic_state
>     instead of skl_ddb_values
>     (to be nuked in Villes patch)
> 
> v3: Renamed "enabled_slices" to "enabled_dbuf_slices_num"
>     (Matt Roper)
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 38 ++++++++++++++------
>  1 file changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 62e33bca7014..0e09d0c23b1d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14546,13 +14546,33 @@ static void intel_update_trans_port_sync_crtcs(struct intel_crtc *crtc,
>  				       state);
>  }
>  
> +static void icl_dbuf_slice_pre_update(struct intel_atomic_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	u8 hw_enabled_slices = dev_priv->enabled_dbuf_slices_num;
> +	u8 required_slices = state->enabled_dbuf_slices_num;
> +
> +	/* If 2nd DBuf slice required, enable it here */
> +	if (INTEL_GEN(dev_priv) >= 11 && required_slices > hw_enabled_slices)
> +		icl_dbuf_slices_update(dev_priv, required_slices);
> +}
> +
> +static void icl_dbuf_slice_post_update(struct intel_atomic_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	u8 hw_enabled_slices = dev_priv->enabled_dbuf_slices_num;
> +	u8 required_slices = state->enabled_dbuf_slices_num;
> +
> +	/* If 2nd DBuf slice is no more required disable it */
> +	if (INTEL_GEN(dev_priv) >= 11 && required_slices < hw_enabled_slices)
> +		icl_dbuf_slices_update(dev_priv, required_slices);
> +}
> +
>  static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>  	struct intel_crtc *crtc;
>  	struct intel_crtc_state *old_crtc_state, *new_crtc_state;
> -	u8 hw_enabled_slices = dev_priv->enabled_dbuf_slices_num;
> -	u8 required_slices = state->enabled_dbuf_slices_num;
>  	struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
>  	u8 dirty_pipes = 0;
>  	int i;
> @@ -14565,10 +14585,6 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  			dirty_pipes |= BIT(crtc->pipe);
>  	}
>  
> -	/* If 2nd DBuf slice required, enable it here */
> -	if (INTEL_GEN(dev_priv) >= 11 && required_slices > hw_enabled_slices)
> -		icl_dbuf_slices_update(dev_priv, required_slices);
> -
>  	/*
>  	 * Whenever the number of active pipes changes, we need to make sure we
>  	 * update the pipes in the right order so that their ddb allocations
> @@ -14617,10 +14633,6 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>  				intel_wait_for_vblank(dev_priv, pipe);
>  		}
>  	}
> -
> -	/* If 2nd DBuf slice is no more required disable it */
> -	if (INTEL_GEN(dev_priv) >= 11 && required_slices < hw_enabled_slices)
> -		icl_dbuf_slices_update(dev_priv, required_slices);
>  }
>  
>  static void intel_atomic_helper_free_state(struct drm_i915_private *dev_priv)
> @@ -14750,6 +14762,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  	if (state->modeset)
>  		intel_encoders_update_prepare(state);
>  
> +	/* Enable all new slices, we might need */
> +	icl_dbuf_slice_pre_update(state);
> +
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>  	dev_priv->display.commit_modeset_enables(state);
>  
> @@ -14825,6 +14840,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  	if (state->modeset && intel_can_enable_sagv(state))
>  		intel_enable_sagv(dev_priv);
>  
> +	/* Disable all slices, we don't need */
> +	icl_dbuf_slice_post_update(state);

I would put that just before or after the .optimize_watermarks() loop.
That's kinda the equivalent spot where we do other FIFO related things
on older platforms. And crucially it's before we do the extra underrun
checks and state verification. Not sure why the state verification isn't
blowing up for you with this actually?

Otherwise looks good
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>


> +
>  	drm_atomic_helper_commit_hw_done(&state->base);
>  
>  	if (state->modeset) {
> -- 
> 2.17.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx