Re: [PATCH 2/2] drm/i915: Only recalculate wm's for planes part of the state, v2.

[Matt Roper <matthew.d.roper@intel.com>]

On Tue, Mar 01, 2016 at 11:07:22AM +0100, Maarten Lankhorst wrote:
> Only planes that are part of the state should be used for recalculating
> watermarks. For planes not part of the state the previous patch allows
> us to re-use the old values since they're calculated even for levels
> that are not actively used.
> 
> Changes since v1:
> - Remove big if from intel_crtc_atomic_check.
> - Remove extra newline.
> - Remove memset in ilk_compute_pipe_wm.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>

I haven't thought through this too carefully yet, but off the top of my
head I'm not sure if this will work for SKL once we transition it to
also use a more atomic style.  Changes to other state might result in
changes to DDB allocation, making our previously-calculated values
invalid.

I think this is okay for the current code (where only ILK is atomic), so

    Acknowledged-by: Matt Roper <matthew.d.roper@intel.com>

for the time being.  I'll be back to looking at SKL-style watermarks in
the next day or two and I might have to backtrack somewhat in cases
where a DDB partitioning changes results in a full recompute, but I need
to think through the details a bit more about how best to handle that.


Matt


> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  3 +-
>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h     | 12 ++++++++
>  drivers/gpu/drm/i915/intel_pm.c      | 59 ++++++++++++++++++++----------------
>  4 files changed, 47 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 671295523317..4b8f446cdf44 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -629,8 +629,7 @@ struct drm_i915_display_funcs {
>  			  int target, int refclk,
>  			  struct dpll *match_clock,
>  			  struct dpll *best_clock);
> -	int (*compute_pipe_wm)(struct intel_crtc *crtc,
> -			       struct drm_atomic_state *state);
> +	int (*compute_pipe_wm)(struct intel_crtc_state *cstate);
>  	int (*compute_intermediate_wm)(struct drm_device *dev,
>  				       struct intel_crtc *intel_crtc,
>  				       struct intel_crtc_state *newstate);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 79bf527e0a73..20a4cb3f69d1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11994,7 +11994,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  
>  	ret = 0;
>  	if (dev_priv->display.compute_pipe_wm) {
> -		ret = dev_priv->display.compute_pipe_wm(intel_crtc, state);
> +		ret = dev_priv->display.compute_pipe_wm(pipe_config);
>  		if (ret) {
>  			DRM_DEBUG_KMS("Target pipe watermarks are invalid\n");
>  			return ret;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5daf53c080e1..03b7d031e910 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1630,6 +1630,18 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state,
>  
>  	return to_intel_crtc_state(crtc_state);
>  }
> +
> +static inline struct intel_plane_state *
> +intel_atomic_get_existing_plane_state(struct drm_atomic_state *state,
> +				      struct intel_plane *plane)
> +{
> +	struct drm_plane_state *plane_state;
> +
> +	plane_state = drm_atomic_get_existing_plane_state(state, &plane->base);
> +
> +	return to_intel_plane_state(plane_state);
> +}
> +
>  int intel_atomic_setup_scalers(struct drm_device *dev,
>  	struct intel_crtc *intel_crtc,
>  	struct intel_crtc_state *crtc_state);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1b8ba777d2b3..dc1e1683f4a3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1996,11 +1996,18 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
>  		cur_latency *= 5;
>  	}
>  
> -	result->pri_val = ilk_compute_pri_wm(cstate, pristate,
> -					     pri_latency, level);
> -	result->spr_val = ilk_compute_spr_wm(cstate, sprstate, spr_latency);
> -	result->cur_val = ilk_compute_cur_wm(cstate, curstate, cur_latency);
> -	result->fbc_val = ilk_compute_fbc_wm(cstate, pristate, result->pri_val);
> +	if (pristate) {
> +		result->pri_val = ilk_compute_pri_wm(cstate, pristate,
> +						     pri_latency, level);
> +		result->fbc_val = ilk_compute_fbc_wm(cstate, pristate, result->pri_val);
> +	}
> +
> +	if (sprstate)
> +		result->spr_val = ilk_compute_spr_wm(cstate, sprstate, spr_latency);
> +
> +	if (curstate)
> +		result->cur_val = ilk_compute_cur_wm(cstate, curstate, cur_latency);
> +
>  	result->enable = true;
>  }
>  
> @@ -2288,51 +2295,51 @@ static bool ilk_validate_pipe_wm(struct drm_device *dev,
>  }
>  
>  /* Compute new watermarks for the pipe */
> -static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
> -			       struct drm_atomic_state *state)
> +static int ilk_compute_pipe_wm(struct intel_crtc_state *cstate)
>  {
> +	struct drm_atomic_state *state = cstate->base.state;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
>  	struct intel_pipe_wm *pipe_wm;
> -	struct drm_device *dev = intel_crtc->base.dev;
> +	struct drm_device *dev = state->dev;
>  	const struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc_state *cstate = NULL;
>  	struct intel_plane *intel_plane;
> -	struct drm_plane_state *ps;
>  	struct intel_plane_state *pristate = NULL;
>  	struct intel_plane_state *sprstate = NULL;
>  	struct intel_plane_state *curstate = NULL;
>  	int level, max_level = ilk_wm_max_level(dev), usable_level;
>  	struct ilk_wm_maximums max;
>  
> -	cstate = intel_atomic_get_crtc_state(state, intel_crtc);
> -	if (IS_ERR(cstate))
> -		return PTR_ERR(cstate);
> -
>  	pipe_wm = &cstate->wm.optimal.ilk;
>  
>  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> -		ps = drm_atomic_get_plane_state(state,
> -						&intel_plane->base);
> -		if (IS_ERR(ps))
> -			return PTR_ERR(ps);
> +		struct intel_plane_state *ps;
> +
> +		ps = intel_atomic_get_existing_plane_state(state,
> +							   intel_plane);
> +		if (!ps)
> +			continue;
>  
>  		if (intel_plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> -			pristate = to_intel_plane_state(ps);
> +			pristate = ps;
>  		else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY)
> -			sprstate = to_intel_plane_state(ps);
> +			sprstate = ps;
>  		else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
> -			curstate = to_intel_plane_state(ps);
> +			curstate = ps;
>  	}
>  
>  	pipe_wm->pipe_enabled = cstate->base.active;
> -	pipe_wm->sprites_enabled = sprstate->visible;
> -	pipe_wm->sprites_scaled = sprstate->visible &&
> -		(drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
> -		drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16);
> +	if (sprstate) {
> +		pipe_wm->sprites_enabled = sprstate->visible;
> +		pipe_wm->sprites_scaled = sprstate->visible &&
> +			(drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
> +			 drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16);
> +	}
> +
>  
>  	usable_level = max_level;
>  
>  	/* ILK/SNB: LP2+ watermarks only w/o sprites */
> -	if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
> +	if (INTEL_INFO(dev)->gen <= 6 && pipe_wm->sprites_enabled)
>  		usable_level = 1;
>  
>  	/* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
> -- 
> 2.1.0
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx