Re: [isg-gms] [RFC 3/6] drm/i915/vlv: Move fifo_size from intel_plane_wm_parameters to vlv_wm_state

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

On Wed, Jun 08, 2016 at 05:22:41PM +0200, Chi Ding wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> This commit saves watermark for each plane in vlv_wm_state to prepare
> for two-level watermark because we'll compute and save intermediate and
> optimal watermark and fifo size for each plane.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Chi Ding <chix.ding@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  12 +----
>  drivers/gpu/drm/i915/intel_pm.c  | 111 +++++++++++++++++++++------------------
>  2 files changed, 61 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index b973b86..31118e1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -624,6 +624,7 @@ struct intel_crtc_state {
>  struct vlv_wm_state {
>  	struct vlv_pipe_wm wm[3];
>  	struct vlv_sr_wm sr[3];
> +	uint16_t fifo_size[I915_MAX_PLANES];
>  	uint8_t num_active_planes;
>  	uint8_t num_levels;
>  	uint8_t level;
> @@ -696,10 +697,6 @@ struct intel_crtc {
>  	struct vlv_wm_state wm_state;
>  };
>  
> -struct intel_plane_wm_parameters {
> -	uint16_t fifo_size;
> -};
> -
>  struct intel_plane {
>  	struct drm_plane base;
>  	int plane;
> @@ -708,13 +705,6 @@ struct intel_plane {
>  	int max_downscale;
>  	uint32_t frontbuffer_bit;
>  
> -	/* Since we need to change the watermarks before/after
> -	 * enabling/disabling the planes, we need to store the parameters here
> -	 * as the other pieces of the struct may not reflect the values we want
> -	 * for the watermark calculations. Currently only Haswell uses this.
> -	 */
> -	struct intel_plane_wm_parameters wm;
> -
>  	/*
>  	 * NOTE: Do not place new plane state fields here (e.g., when adding
>  	 * new plane properties).  New runtime state should now be placed in
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a3942df..33f52ae 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -983,14 +983,16 @@ static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
>  	return min_t(int, wm, USHRT_MAX);
>  }
>  
> -static void vlv_compute_fifo(struct intel_crtc *crtc)
> +static void vlv_compute_fifo(struct intel_crtc *crtc,
> +				struct vlv_wm_state *wm_state)
>  {
>  	struct drm_device *dev = crtc->base.dev;
> -	struct vlv_wm_state *wm_state = &crtc->wm_state;
>  	struct intel_plane *plane;
>  	unsigned int total_rate = 0;
>  	const int fifo_size = 512 - 1;
>  	int fifo_extra, fifo_left = fifo_size;
> +	int rate[I915_MAX_PLANES] = {};

I think this syntax might cause a warning on some versions of GCC (iirc,
empty braces are technically illegal in the C spec, but legal in C++).
I believe providing the value of the first element will avoid the
warning (and still initialize all entries to 0); i.e., 

        int rate[I915_MAX_PLANES] = { 0 };

> +	int i;
>  
>  	for_each_intel_plane_on_crtc(dev, crtc, plane) {
>  		struct intel_plane_state *state =
> @@ -1001,58 +1003,55 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
>  
>  		if (state->visible) {
>  			wm_state->num_active_planes++;
> -			total_rate += drm_format_plane_cpp(state->base.fb->pixel_format, 0);
> +			rate[wm_plane_id(plane)] =
> +			drm_format_plane_cpp(state->base.fb->pixel_format, 0);
> +			total_rate += rate[wm_plane_id(plane)];
>  		}
>  	}
>  
> -	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> -		struct intel_plane_state *state =
> -			to_intel_plane_state(plane->base.state);
> -		unsigned int rate;
> -
> -		if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
> -			plane->wm.fifo_size = 63;
> +	for (i = 0; i < I915_MAX_PLANES; i++) {

Is there a specific reason to change from iterating over planes to
iterating over indices here?  I think the end result is the same either
way as far as I can see?  (Assuming you could just set
i = wm_plane_id(plane) like you did in the first loop if you kept the
original loop).


> +		if (i == PLANE_CURSOR) {
> +			wm_state->fifo_size[i] = 63;
>  			continue;
>  		}
>  
> -		if (!state->visible) {
> -			plane->wm.fifo_size = 0;
> +		if (!rate[i]) {
> +			wm_state->fifo_size[i] = 0;
>  			continue;
>  		}
>  
> -		rate = drm_format_plane_cpp(state->base.fb->pixel_format, 0);
> -		plane->wm.fifo_size = fifo_size * rate / total_rate;
> -		fifo_left -= plane->wm.fifo_size;
> +		wm_state->fifo_size[i] = fifo_size * rate[i] / total_rate;
> +		fifo_left -= wm_state->fifo_size[i];
>  	}
>  
>  	fifo_extra = DIV_ROUND_UP(fifo_left, wm_state->num_active_planes ?: 1);
>  
>  	/* spread the remainder evenly */
> -	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> +	for (i = 0; i < I915_MAX_PLANES; i++) {
>  		int plane_extra;
>  
>  		if (fifo_left == 0)
>  			break;
>  
> -		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
> +		if (i == PLANE_CURSOR)
>  			continue;
>  
>  		/* give it all to the first plane if none are active */
> -		if (plane->wm.fifo_size == 0 &&
> +		if (!wm_state->fifo_size[i] &&
>  		    wm_state->num_active_planes)
>  			continue;
>  
>  		plane_extra = min(fifo_extra, fifo_left);
> -		plane->wm.fifo_size += plane_extra;
> +		wm_state->fifo_size[i] += plane_extra;
>  		fifo_left -= plane_extra;
>  	}
>  
>  	WARN_ON(fifo_left != 0);
>  }
>  
> -static void vlv_invert_wms(struct intel_crtc *crtc)
> +static void vlv_invert_wms(struct intel_crtc *crtc,
> +			struct vlv_wm_state *wm_state)
>  {
> -	struct vlv_wm_state *wm_state = &crtc->wm_state;

Passing wm_state by parameter seems unrelated to the purpose of this
patch (moving the fifo_size field).  Was it supposed to go in a later
patch?

>  	int level;
>  
>  	for (level = 0; level < wm_state->num_levels; level++) {
> @@ -1064,19 +1063,24 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
>  		wm_state->sr[level].cursor = 63 - wm_state->sr[level].cursor;
>  
>  		for_each_intel_plane_on_crtc(dev, crtc, plane) {
> +			int i = wm_plane_id(plane);
> +
>  			switch (plane->base.type) {
>  				int sprite;
>  			case DRM_PLANE_TYPE_CURSOR:
> -				wm_state->wm[level].cursor = plane->wm.fifo_size -
> +				wm_state->wm[level].cursor =
> +					wm_state->fifo_size[i] -
>  					wm_state->wm[level].cursor;
>  				break;
>  			case DRM_PLANE_TYPE_PRIMARY:
> -				wm_state->wm[level].primary = plane->wm.fifo_size -
> +				wm_state->wm[level].primary =
> +					wm_state->fifo_size[i] -
>  					wm_state->wm[level].primary;
>  				break;
>  			case DRM_PLANE_TYPE_OVERLAY:
>  				sprite = plane->plane;
> -				wm_state->wm[level].sprite[sprite] = plane->wm.fifo_size -
> +				wm_state->wm[level].sprite[sprite] =
> +					wm_state->fifo_size[i] -
>  					wm_state->wm[level].sprite[sprite];
>  				break;
>  			}
> @@ -1084,7 +1088,7 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
>  	}
>  }
>  
> -static void vlv_compute_wm(struct intel_crtc *crtc)
> +static int vlv_compute_wm(struct intel_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct vlv_wm_state *wm_state = &crtc->wm_state;
> @@ -1099,7 +1103,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
>  
>  	wm_state->num_active_planes = 0;
>  
> -	vlv_compute_fifo(crtc);
> +	vlv_compute_fifo(crtc, wm_state);
>  
>  	if (wm_state->num_active_planes != 1)
>  		wm_state->cxsr = false;
> @@ -1123,11 +1127,16 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
>  			int wm = vlv_compute_wm_level(plane, crtc, state, level);
>  			int max_wm = plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
>  
> -			/* hack */
> -			if (WARN_ON(level == 0 && wm > max_wm))
> -				wm = max_wm;
> +			if (level == 0 && wm > max_wm) {
> +				DRM_DEBUG_KMS("Requested display configuration "
> +				"exceeds system watermark limitations\n");
> +				DRM_DEBUG_KMS("Plane %d.%d: blocks required = %u/%u\n",
> +				      crtc->pipe,
> +				      drm_plane_index(&plane->base), wm, max_wm);
> +				return -EINVAL;
> +			}

This is an important change, but I think you meant to have this land in
a different patch since it's unrelated to the content of this patch
(which simply moves the fifo_size field).


>  
> -			if (wm > plane->wm.fifo_size)
> +			if (wm > wm_state->fifo_size[wm_plane_id(plane)])
>  				break;
>  
>  			switch (plane->base.type) {
> @@ -1180,7 +1189,9 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
>  		memset(&wm_state->sr[level], 0, sizeof(wm_state->sr[level]));
>  	}
>  
> -	vlv_invert_wms(crtc);
> +	vlv_invert_wms(crtc, wm_state);
> +
> +	return 0;
>  }
>  
>  #define VLV_FIFO(plane, value) \
> @@ -1190,24 +1201,18 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_plane *plane;
>  	int sprite0_start = 0, sprite1_start = 0, fifo_size = 0;
> +	const struct vlv_wm_state *wm_state = &crtc->wm_state;
>  
> -	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> -		if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
> -			WARN_ON(plane->wm.fifo_size != 63);
> -			continue;
> -		}
>  
> -		if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> -			sprite0_start = plane->wm.fifo_size;
> -		else if (plane->plane == 0)
> -			sprite1_start = sprite0_start + plane->wm.fifo_size;
> -		else
> -			fifo_size = sprite1_start + plane->wm.fifo_size;
> -	}
> +	WARN_ON(wm_state->fifo_size[PLANE_CURSOR] != 63);
> +	sprite0_start = wm_state->fifo_size[0];
> +	sprite1_start = sprite0_start + wm_state->fifo_size[1];
> +	fifo_size = sprite1_start + wm_state->fifo_size[2];
>  
> -	WARN_ON(fifo_size != 512 - 1);
> +	WARN(fifo_size != 512 - 1, "Pipe %c FIFO split %d / %d / %d\n",
> +		      pipe_name(crtc->pipe), sprite0_start,
> +		      sprite1_start, fifo_size);

The DRM_DEBUG_KMS() call below gives the same info you're adding to the
message here; if a developer is debugging a problem here, I assume
they'll be running with drm.debug=0xf or similar, so do we really need
to change the WARN() line here to duplicate that info?


Matt

>  
>  	DRM_DEBUG_KMS("Pipe %c FIFO split %d / %d / %d\n",
>  		      pipe_name(crtc->pipe), sprite0_start,
> @@ -4216,6 +4221,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct vlv_wm_values *wm = &dev_priv->wm.vlv;
> +	struct intel_crtc *crtc;
>  	struct intel_plane *plane;
>  	enum pipe pipe;
>  	u32 val;
> @@ -4223,17 +4229,20 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>  	vlv_read_wm_values(dev_priv, wm);
>  
>  	for_each_intel_plane(dev, plane) {
> +		struct vlv_wm_state *wm_state;
> +		int i = wm_plane_id(plane);
> +
> +		crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev, plane->pipe));
> +		wm_state = &crtc->wm_state;
> +
>  		switch (plane->base.type) {
> -			int sprite;
>  		case DRM_PLANE_TYPE_CURSOR:
> -			plane->wm.fifo_size = 63;
> +			wm_state->fifo_size[i] = 63;
>  			break;
>  		case DRM_PLANE_TYPE_PRIMARY:
> -			plane->wm.fifo_size = vlv_get_fifo_size(dev, plane->pipe, 0);
> -			break;
>  		case DRM_PLANE_TYPE_OVERLAY:
> -			sprite = plane->plane;
> -			plane->wm.fifo_size = vlv_get_fifo_size(dev, plane->pipe, sprite + 1);
> +			wm_state->fifo_size[i] =
> +				vlv_get_fifo_size(dev, plane->pipe, i);
>  			break;
>  		}
>  	}
> -- 
> 1.8.0.1
> 
> -------------------------------------
> isg-gms@eclists.intel.com
> https://eclists.intel.com/sympa/info/isg-gms
> Unsubscribe by sending email to sympa@eclists.intel.com with subject "Unsubscribe isg-gms"

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