Re: [Intel-gfx] [PATCH 2/6] drm/i915: Nuke intel_atomic_crtc_state_for_each_plane_state() from skl+ wm code

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Fri, Nov 06, 2020 at 07:30:38PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> intel_atomic_crtc_state_for_each_plane_state() peeks at the
> plane's current state without holding the plane's mutex, trusting
> that the crtc's mutex will protect it. In practice that does work
> since our planes can't move between pipes, but it sets a bad
> example. intel_atomic_crtc_state_for_each_plane_state() also
> relies on crtc_state.uapi.plane_mask which may be full of lies
> when it comes to the bigjoiner stuff, so soon we can't use it as
> is anyway. So best to just get rid of it entirely. Which we can
> easily do by switching to the g4x/vlv "raw" watermark approach.
> 
> Later on we should even be able to move the "raw" watermark
> computation into the normal .plane_check() code, leaving only
> the merging/clamping of the final watermarks to the later
> stages. But that will require adjusting the ilk+ wm code
> similarly as well.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

> ---
>  .../drm/i915/display/intel_display_types.h    |  2 +
>  drivers/gpu/drm/i915/intel_pm.c               | 41 +++++++++++--------
>  2 files changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index f6f0626649e0..6b249969c394 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -755,6 +755,8 @@ struct intel_crtc_wm_state {
>  		} ilk;
>  
>  		struct {
> +			/* "raw" watermarks */
> +			struct skl_pipe_wm raw;
>  			/* gen9+ only needs 1-step wm programming */
>  			struct skl_pipe_wm optimal;
>  			struct skl_ddb_entry ddb;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index e9ac6f1a5d28..85b4bfb02e2e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5480,7 +5480,7 @@ static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state,
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	struct skl_plane_wm *wm = &crtc_state->wm.skl.optimal.planes[plane_id];
> +	struct skl_plane_wm *wm = &crtc_state->wm.skl.raw.planes[plane_id];
>  	struct skl_wm_params wm_params;
>  	int ret;
>  
> @@ -5503,7 +5503,7 @@ static int skl_build_plane_wm_uv(struct intel_crtc_state *crtc_state,
>  				 const struct intel_plane_state *plane_state,
>  				 enum plane_id plane_id)
>  {
> -	struct skl_plane_wm *wm = &crtc_state->wm.skl.optimal.planes[plane_id];
> +	struct skl_plane_wm *wm = &crtc_state->wm.skl.raw.planes[plane_id];
>  	struct skl_wm_params wm_params;
>  	int ret;
>  
> @@ -5524,10 +5524,13 @@ static int skl_build_plane_wm(struct intel_crtc_state *crtc_state,
>  			      const struct intel_plane_state *plane_state)
>  {
>  	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> -	const struct drm_framebuffer *fb = plane_state->hw.fb;
>  	enum plane_id plane_id = plane->id;
> +	struct skl_plane_wm *wm = &crtc_state->wm.skl.raw.planes[plane_id];
> +	const struct drm_framebuffer *fb = plane_state->hw.fb;
>  	int ret;
>  
> +	memset(wm, 0, sizeof(*wm));
> +
>  	if (!intel_wm_plane_visible(crtc_state, plane_state))
>  		return 0;
>  
> @@ -5549,10 +5552,14 @@ static int skl_build_plane_wm(struct intel_crtc_state *crtc_state,
>  static int icl_build_plane_wm(struct intel_crtc_state *crtc_state,
>  			      const struct intel_plane_state *plane_state)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> -	enum plane_id plane_id = to_intel_plane(plane_state->uapi.plane)->id;
> +	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +	enum plane_id plane_id = plane->id;
> +	struct skl_plane_wm *wm = &crtc_state->wm.skl.raw.planes[plane_id];
>  	int ret;
>  
> +	memset(wm, 0, sizeof(*wm));
> +
>  	/* Watermarks calculated in master */
>  	if (plane_state->planar_slave)
>  		return 0;
> @@ -5591,19 +5598,18 @@ static int skl_build_pipe_wm(struct intel_atomic_state *state,
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	struct intel_crtc_state *crtc_state =
>  		intel_atomic_get_new_crtc_state(state, crtc);
> -	struct skl_pipe_wm *pipe_wm = &crtc_state->wm.skl.optimal;
> -	struct intel_plane *plane;
>  	const struct intel_plane_state *plane_state;
> -	int ret;
> +	struct intel_plane *plane;
> +	int ret, i;
>  
> -	/*
> -	 * We'll only calculate watermarks for planes that are actually
> -	 * enabled, so make sure all other planes are set as disabled.
> -	 */
> -	memset(pipe_wm->planes, 0, sizeof(pipe_wm->planes));
> -
> -	intel_atomic_crtc_state_for_each_plane_state(plane, plane_state,
> -						     crtc_state) {
> +	for_each_new_intel_plane_in_state(state, plane, plane_state, i) {
> +		/*
> +		 * FIXME should perhaps check {old,new}_plane_crtc->hw.crtc
> +		 * instead but we don't populate that correctly for NV12 Y
> +		 * planes so for now hack this.
> +		 */
> +		if (plane->pipe != crtc->pipe)
> +			continue;
>  
>  		if (INTEL_GEN(dev_priv) >= 11)
>  			ret = icl_build_plane_wm(crtc_state, plane_state);
> @@ -5613,6 +5619,8 @@ static int skl_build_pipe_wm(struct intel_atomic_state *state,
>  			return ret;
>  	}
>  
> +	crtc_state->wm.skl.optimal = crtc_state->wm.skl.raw;
> +
>  	return 0;
>  }
>  
> @@ -6273,6 +6281,7 @@ void skl_wm_get_hw_state(struct drm_i915_private *dev_priv)
>  		crtc_state = to_intel_crtc_state(crtc->base.state);
>  
>  		skl_pipe_wm_get_hw_state(crtc, &crtc_state->wm.skl.optimal);
> +		crtc_state->wm.skl.raw = crtc_state->wm.skl.optimal;
>  	}
>  
>  	if (dev_priv->active_pipes) {
> -- 
> 2.26.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx