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

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

[Matt Roper]

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

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

[Maarten Lankhorst]

Op 14-06-16 om 23:52 schreef Matt Roper:
> 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., 
Kernel allows it, just grep for '= { }' or '= {}'.
>         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).
Yeah, result is the same, but this way we don't have to grab the plane state multiple times.
>
>> +		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?
Maybe, but it doesn't change much. The same wm_state is still used.
>>  	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).
Agreed.
>
>>  
>> -			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?

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

Re: [RFC 4/6] drm/i915/vlv: Change to use intel_crtc_state instead of base CRTC object

[Matt Roper]

On Wed, Jun 08, 2016 at 05:22:42PM +0200, Chi Ding wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> This commit changs some functions to operate on intel_crtc_state rather
> than the base CRTC objects in order to transit to atomic

I'd add a little bit more justification to this message.

The reason we want to do this is to allow future patches to move the
computation steps into the atomic 'check' phase where they'll be
operating on in-flight CRTC states rather than already-committed states.

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Chi Ding <chix.ding@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 33f52ae..3fc80f9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -983,9 +983,10 @@ 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_state *cstate,
>  				struct vlv_wm_state *wm_state)
>  {
> +	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
>  	struct drm_device *dev = crtc->base.dev;
>  	struct intel_plane *plane;
>  	unsigned int total_rate = 0;
> @@ -1088,8 +1089,9 @@ static void vlv_invert_wms(struct intel_crtc *crtc,
>  	}
>  }
>  
> -static int vlv_compute_wm(struct intel_crtc *crtc)
> +static int vlv_compute_wm(struct intel_crtc_state *cstate)
>  {
> +	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
>  	struct drm_device *dev = crtc->base.dev;
>  	struct vlv_wm_state *wm_state = &crtc->wm_state;
>  	struct intel_plane *plane;
> @@ -1103,7 +1105,7 @@ static int vlv_compute_wm(struct intel_crtc *crtc)
>  
>  	wm_state->num_active_planes = 0;
>  
> -	vlv_compute_fifo(crtc, wm_state);
> +	vlv_compute_fifo(cstate, wm_state);
>  
>  	if (wm_state->num_active_planes != 1)
>  		wm_state->cxsr = false;
> @@ -1131,7 +1133,7 @@ static int vlv_compute_wm(struct intel_crtc *crtc)
>  				DRM_DEBUG_KMS("Requested display configuration "
>  				"exceeds system watermark limitations\n");
>  				DRM_DEBUG_KMS("Plane %d.%d: blocks required = %u/%u\n",
> -				      crtc->pipe,
> +				      to_intel_crtc(cstate->base.crtc)->pipe,

This change is unnecessary, right?  'crtc' is still initialized to the
same thing at the top of the function.


Matt

>  				      drm_plane_index(&plane->base), wm, max_wm);
>  				return -EINVAL;
>  			}
> @@ -1332,7 +1334,7 @@ static void vlv_update_wm(struct drm_crtc *crtc)
>  	enum pipe pipe = intel_crtc->pipe;
>  	struct vlv_wm_values wm = {};
>  
> -	vlv_compute_wm(intel_crtc);
> +	vlv_compute_wm(intel_crtc->config);
>  	vlv_merge_wm(dev, &wm);
>  
>  	if (memcmp(&dev_priv->wm.vlv, &wm, sizeof(wm)) == 0) {
> -- 
> 1.8.0.1
> 

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

Re: [isg-gms] [RFC 5/6] drm/i915/vlv: Add optimal field in intel_crtc_wm_state

[Matt Roper]

On Wed, Jun 08, 2016 at 05:22:43PM +0200, Chi Ding wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 

This patch needs a commit message.  Those of us actively working on
watermarks right now understand the end goal for two-stage watermarks,
but I don't think most other developers looking at this code (or
bisecting something to this patch) will immediately understand what
'optimal' means in the context of watermarks (or why we'd ever have
non-optimal watermarks).

> 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 | 23 ++++++++++++++---------
>  drivers/gpu/drm/i915/intel_pm.c  | 11 +++++++++--
>  2 files changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 31118e1..6d2e628 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -420,6 +420,16 @@ struct skl_pipe_wm {
>  	uint32_t linetime;
>  };
>  
> +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;
> +	bool cxsr;
> +};
> +
>  struct intel_crtc_wm_state {
>  	union {
>  		struct {
> @@ -440,6 +450,10 @@ struct intel_crtc_wm_state {
>  		} ilk;
>  
>  		struct {
> +			struct vlv_wm_state optimal;
> +		} vlv;
> +
> +		struct {
>  			/* gen9+ only needs 1-step wm programming */
>  			struct skl_pipe_wm optimal;
>  
> @@ -621,15 +635,6 @@ struct intel_crtc_state {
>  	uint32_t gamma_mode;
>  };
>  
> -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;
> -	bool cxsr;
> -};
>  
>  struct intel_crtc {
>  	struct drm_crtc base;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3fc80f9..502af9f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1093,7 +1093,7 @@ static int vlv_compute_wm(struct intel_crtc_state *cstate)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
>  	struct drm_device *dev = crtc->base.dev;
> -	struct vlv_wm_state *wm_state = &crtc->wm_state;
> +	struct vlv_wm_state *wm_state = &cstate->wm.vlv.optimal;
>  	struct intel_plane *plane;
>  	int sr_fifo_size = INTEL_INFO(dev)->num_pipes * 512 - 1;
>  	int level;
> @@ -1335,6 +1335,7 @@ static void vlv_update_wm(struct drm_crtc *crtc)
>  	struct vlv_wm_values wm = {};
>  
>  	vlv_compute_wm(intel_crtc->config);
> +	intel_crtc->wm_state = intel_crtc->config->wm.vlv.optimal;

Is this now the 'active' set of watermarks?  If so, we might want to
move it into crtc->wm.active.vlv to match the handling we have on other
platforms.

Also, it looks like VLV-style watermarks can still race here.  I think
we need to grab wm_mutex before setting wm_state/active like we do on
the other platforms so that the state doesn't get changed by one thread
while another one is in the middle of merging WM's into final results.

>  	vlv_merge_wm(dev, &wm);
>  
>  	if (memcmp(&dev_priv->wm.vlv, &wm, sizeof(wm)) == 0) {
> @@ -1377,6 +1378,7 @@ static void vlv_update_wm(struct drm_crtc *crtc)
>  		chv_set_memory_dvfs(dev_priv, true);
>  
>  	dev_priv->wm.vlv = wm;
> +

Stray whitespace?


Matt

>  }
>  
>  #define single_plane_enabled(mask) is_power_of_2(mask)
> @@ -4286,10 +4288,15 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>  		mutex_unlock(&dev_priv->rps.hw_lock);
>  	}
>  
> -	for_each_pipe(dev_priv, pipe)
> +	for_each_intel_crtc(dev, crtc) {
> +		pipe = crtc->pipe;
> +		to_intel_crtc_state(crtc->base.state)->wm.vlv.optimal
> +			= crtc->wm_state;
> +
>  		DRM_DEBUG_KMS("Initial watermarks: pipe %c, plane=%d, cursor=%d, sprite0=%d, sprite1=%d\n",
>  			      pipe_name(pipe), wm->pipe[pipe].primary, wm->pipe[pipe].cursor,
>  			      wm->pipe[pipe].sprite[0], wm->pipe[pipe].sprite[1]);
> +	}
>  
>  	DRM_DEBUG_KMS("Initial watermarks: SR plane=%d, SR cursor=%d level=%d cxsr=%d\n",
>  		      wm->sr.plane, wm->sr.cursor, wm->level, wm->cxsr);
> -- 
> 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

Re: [RFC 6/6] drm/i915/vlv: Add intermediate field in intel_crtc_wm_state and handlers for two-level watermark

[Matt Roper]

On Tue, Jun 14, 2016 at 07:40:54AM -0700, Ding, ChiX wrote:
> Hi Maarten
> Thanks for the reply. Please correct me if I'm wrong : 
> If the intermediate watermark is bigger than any of the old and the new, then it's not really safe.
> It could be bigger than the FIFO size. 
> 
> I did some tests using average value and it ran into FIFO underrun, e.g.
> [100424.405624] [drm:vlv_invert_wms] Pipe A fifo for primary, sprite0, sprite 1 : 511 / 0 / 0 
> [100424.413231] [drm:vlv_compute_fifo] Pipe A fifo for primary, sprite0, sprite 1 : 256 / 255 / 0
> [100424.413242] [drm:vlv_compute_pipe_wm] Pipe A
> [100424.413245] [drm:vlv_compute_pipe_wm] level : 0
> [100424.413249] [drm:vlv_compute_pipe_wm] Watermark values for primary, sprite0, sprite1 : 136, 135, 0
> [100424.413253] [drm:vlv_compute_intermediate_wm] level : 0
> [100424.413257] [drm:vlv_compute_intermediate_wm] A watermark values for primary, sprite0, sprite1 : 136, 135, 0
> [100424.413261] [drm:vlv_compute_intermediate_wm] B watermark values for primary, sprite0, sprite1 : 391, 0, 0
> [100424.429823] [drm:vlv_initial_watermarks] vlv_initial_watermarks
> 
> At first, the current active
> fifo sizes 511 / 0 / 0 for primary, sprite0, sprite 1
> watermark 391, 0, 0
> 
> Then the optimal values are calculated : 
> fifo sizes 256 / 255 / 0
> watermarks 136, 135, 0
> 
> The average watermark for primary plane is 391+136/2 = 263, bigger
> than the primary plane fifo size for optimal : 256. It causes underrun
> later on when writing intermediate watermark and the fifo size is
> changed to 256 for primary plane by vlv_pipe_set_fifo_size() function.
> 
> If I use min() for intermediate watermark, I don't run into underrun
> issue in the tests.  My understanding is that with min(), in some
> cases we get 0,0 for watermark , but it's still a legitimate value.
> Please let me know if I'm mistaken.
> 
> 
> Another question: the sum of fifo sizes for primary plane, sprite 0,
> sprite 1 should always be 512-1 = 511.  But the watermarks of the
> planes don't need to add up to 511. They need to stay under or at most
> equal to the respective fifo size, right?

I may be overlooking something since I haven't studied VLV/CHV in as
much detail as the other platforms, but there are basically two parts to
the per-crtc watermark calculations we do during the atomic 'check'
phase.

The first part is slicing up the fifo/DDB between the various planes
that are in use.  The fifo buffer is a fixed size resource made up of
blocks, but it's programmable how much of that buffer is devoted to
driving a specific plane.  The formula we use for slicing up the fifo is
to just give each plane a number of blocks proportional to the fraction
of the total data rate that the plane contributes (with some extra logic
at the end to deal with leftover blocks due to integer division).

The second part is calculating the watermark values for each plane.  The
fifo holds data that's been fetched from RAM for the display controller
to consume; the watermark values (measured in fifo blocks) are basically
telling the hardware how/when it needs to wake up and fetch more data
from RAM into the fifo to ensure the fifo doesn't reach empty.  If you
set the watermark value for a plane too low, then the hardware won't
fetch more data from RAM early enough and the fifo will run dry before
new data from RAM shows up; when this happens, the display controller
doesn't have any data to tell it how to keep driving the display so you
can get various kinds of display corruption (and dmesg warnings about a
FIFO underrun).  Programming watermarks too high is always "fine" from a
correctness point of view --- you're just causing the hardware to wake
up and fetch more data from RAM earlier than it really had to.  You may
be impacting power usage, but you're safe from underruns/corruption.

For intermediate watermarks, we need "worst case" values that satisfy
both the old and new configuration (i.e., won't cause underruns for
either config).  So I think you want to be using max() instead of min()
to calculate your intermediate values.  Once you've calculated the
worst-case (max) values for each plane, I figure you'd want to work
backwards from there and slice up your intermediate per-place fifo
allocation second...I guess you'd probably just set each plane's fifo
size equal to its watermark value to start with, then distribute the
remaining blocks equally between all of the planes that are active in
either new or old configuration (and if you don't have any remaining
blocks, then there's no valid intermediate configuration so we should
probably fail the request).

Again, this is just completely off the top of my head and I haven't
actually looked at the VLV/CHV bspecs lately, so I might be completely
overlooking some important detail about these platforms.


Matt

> 
> Thanks a lot,
> Chi
> 
> 
> 
> 
> 
> 
> 
> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] 
> Sent: Monday, June 13, 2016 10:22 AM
> To: Ding, ChiX; ville.syrjala@linux.intel.com; Roper, Matthew D
> Cc: isg-gms@eclists.intel.com; Adebisi, YetundeX
> Subject: Re: [RFC 6/6] drm/i915/vlv: Add intermediate field in intel_crtc_wm_state and handlers for two-level watermark
> 
> Op 10-06-16 om 18:48 schreef Ding, ChiX:
> > Hi Maarten
> > I've changed the code to use the macro 
> > drm_atomic_crtc_state_for_each_plane_state and the other place to check !new_state->active || modeset in computing intermediate watermark. Test result is good.
> >
> > I have some questions however. 
> > Regarding the logic of computing intermediate watermark, you suggested to use average "
> > wm old:
> > { 0, 511 }
> >
> > wm new:
> > { 511, 0 }
> >
> > min(a, b):
> > { 0, 0 }
> >
> > Averaged:
> > { 255, 255 }; --> only 510 summed, should probably add 1 to the first non-null value since it must add up to with 512 - 1."
> >
> > 1) It looks good to me to use the average when one of the old and new values is 0 as listed in your example.
> > But when none of the old and new are 0 and they're not equal, average 
> > value will be bigger than the smaller value of the two. I thought that the idea of comuting intermediate for VLV is to pick a safe value, i.e. a value <= either of the old and the new?
> No, the average will always be rounded down due to integer math. Not sure how you would end up with higher values if you take the average and divide by 2?
> >
> >
> > 2) About the wm values of the planes add up to 511, I don't have the 
> > spec of watermark for VLV but I noticed that it doesn't seem like the 
> > case from dmesg output without the two-level watermark patches Example 1 :
> >  [13634.545172] [drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 
> > 511 / 511  [13634.545180] [drm:vlv_update_wm] Setting FIFO watermarks 
> > - A: plane=0, cursor=0, sprite0=0, sprite1=0, SR: plane=0, cursor=0 
> > level=0 cxsr=0
> >
> > Example 2:
> >  [13635.127046] [drm:vlv_pipe_set_fifo_size] Pipe A FIFO split 511 / 
> > 511 / 511  [13635.127053] [drm:vlv_update_wm] Setting FIFO watermarks 
> > - A: plane=421, cursor=63, sprite0=0, sprite1=0, SR: plane=0, cursor=0 
> > level=0 cxsr=0
> Look at vlv_compute_fifo, the last loop adds the remainder to the first active plane, or when none are active puts it all on the primary plane.
> >
> > 3) I see that in vlv_wm_get_hw_state(), it assigns the current 
> > vlv_wm_state to the optimal, for_each_intel_crtc(dev, crtc) {
> > 	pipe = crtc->pipe;
> > 	to_intel_crtc_state(crtc->base.state)->wm.vlv.optimal
> > 		= crtc->wm_state;
> >
> > I thought that the optimal is computed in intel_crtc_atomic_check() by 
> > .compute_pipe_wm() handler Do we need to assign the current value to it in  vlv_wm_get_hw_state(), which is called by intel_modeset_setup_hw_state()?
> This is a get_hw_state function. It's only used by hw readout during driver init or resume.

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

Re: [RFC 6/6] drm/i915/vlv: Add intermediate field in intel_crtc_wm_state and handlers for two-level watermark

[Ville Syrjälä]

On Tue, Jun 14, 2016 at 03:57:28PM -0700, Matt Roper wrote:
> On Tue, Jun 14, 2016 at 07:40:54AM -0700, Ding, ChiX wrote:
> > Hi Maarten
> > Thanks for the reply. Please correct me if I'm wrong : 
> > If the intermediate watermark is bigger than any of the old and the new, then it's not really safe.
> > It could be bigger than the FIFO size. 
> > 
> > I did some tests using average value and it ran into FIFO underrun, e.g.
> > [100424.405624] [drm:vlv_invert_wms] Pipe A fifo for primary, sprite0, sprite 1 : 511 / 0 / 0 
> > [100424.413231] [drm:vlv_compute_fifo] Pipe A fifo for primary, sprite0, sprite 1 : 256 / 255 / 0
> > [100424.413242] [drm:vlv_compute_pipe_wm] Pipe A
> > [100424.413245] [drm:vlv_compute_pipe_wm] level : 0
> > [100424.413249] [drm:vlv_compute_pipe_wm] Watermark values for primary, sprite0, sprite1 : 136, 135, 0
> > [100424.413253] [drm:vlv_compute_intermediate_wm] level : 0
> > [100424.413257] [drm:vlv_compute_intermediate_wm] A watermark values for primary, sprite0, sprite1 : 136, 135, 0
> > [100424.413261] [drm:vlv_compute_intermediate_wm] B watermark values for primary, sprite0, sprite1 : 391, 0, 0
> > [100424.429823] [drm:vlv_initial_watermarks] vlv_initial_watermarks
> > 
> > At first, the current active
> > fifo sizes 511 / 0 / 0 for primary, sprite0, sprite 1
> > watermark 391, 0, 0
> > 
> > Then the optimal values are calculated : 
> > fifo sizes 256 / 255 / 0
> > watermarks 136, 135, 0
> > 
> > The average watermark for primary plane is 391+136/2 = 263, bigger
> > than the primary plane fifo size for optimal : 256. It causes underrun
> > later on when writing intermediate watermark and the fifo size is
> > changed to 256 for primary plane by vlv_pipe_set_fifo_size() function.
> > 
> > If I use min() for intermediate watermark, I don't run into underrun
> > issue in the tests.  My understanding is that with min(), in some
> > cases we get 0,0 for watermark , but it's still a legitimate value.
> > Please let me know if I'm mistaken.
> > 
> > 
> > Another question: the sum of fifo sizes for primary plane, sprite 0,
> > sprite 1 should always be 512-1 = 511.  But the watermarks of the
> > planes don't need to add up to 511. They need to stay under or at most
> > equal to the respective fifo size, right?
> 
> I may be overlooking something since I haven't studied VLV/CHV in as
> much detail as the other platforms, but there are basically two parts to
> the per-crtc watermark calculations we do during the atomic 'check'
> phase.
> 
> The first part is slicing up the fifo/DDB between the various planes
> that are in use.  The fifo buffer is a fixed size resource made up of
> blocks, but it's programmable how much of that buffer is devoted to
> driving a specific plane.  The formula we use for slicing up the fifo is
> to just give each plane a number of blocks proportional to the fraction
> of the total data rate that the plane contributes (with some extra logic
> at the end to deal with leftover blocks due to integer division).
> 
> The second part is calculating the watermark values for each plane.  The
> fifo holds data that's been fetched from RAM for the display controller
> to consume; the watermark values (measured in fifo blocks) are basically
> telling the hardware how/when it needs to wake up and fetch more data
> from RAM into the fifo to ensure the fifo doesn't reach empty.  If you
> set the watermark value for a plane too low, then the hardware won't
> fetch more data from RAM early enough and the fifo will run dry before
> new data from RAM shows up; when this happens, the display controller
> doesn't have any data to tell it how to keep driving the display so you
> can get various kinds of display corruption (and dmesg warnings about a
> FIFO underrun).  Programming watermarks too high is always "fine" from a
> correctness point of view --- you're just causing the hardware to wake
> up and fetch more data from RAM earlier than it really had to.  You may
> be impacting power usage, but you're safe from underruns/corruption.
> 
> For intermediate watermarks, we need "worst case" values that satisfy
> both the old and new configuration (i.e., won't cause underruns for
> either config).  So I think you want to be using max() instead of min()
> to calculate your intermediate values.  Once you've calculated the
> worst-case (max) values for each plane, I figure you'd want to work
> backwards from there and slice up your intermediate per-place fifo
> allocation second...I guess you'd probably just set each plane's fifo
> size equal to its watermark value to start with, then distribute the
> remaining blocks equally between all of the planes that are active in
> either new or old configuration (and if you don't have any remaining
> blocks, then there's no valid intermediate configuration so we should
> probably fail the request).
> 
> Again, this is just completely off the top of my head and I haven't
> actually looked at the VLV/CHV bspecs lately, so I might be completely
> overlooking some important detail about these platforms.

There won't be any intermediate FIFO sizes. The FIFO repartitioning
happens atomically with plane updates, or rather should, but the exact
nature of the register double buffering is still a bit of a mystery.
How exactly the double buffering works may dictate how we have to do
things, so until someone figures it out, I don't think there's any
point in trying to fix the watermark updates.

step 1. figure out the DSBARB double buffering
step 2. ?
step 3. profit

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

Re: [RFC 6/6] drm/i915/vlv: Add intermediate field in intel_crtc_wm_state and handlers for two-level watermark

[Ding, ChiX]

The watermark means that how much is consumed from a full fifo for VLV/CHV.
From VLV spec, "Display Plane A FIFO Watermark: Number in 64Bs of space in FIFO above which the Display A Stream
will generate requests to Memory"
It seems that smaller value should be picked to avoid FIFO underrun when computing intermediate watermark.

There is a page about DSPARB Display Arbitration Control register for VLV/CHV
https://gfxspecs.intel.com/Predator/Home/Index/16287




-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Wednesday, June 15, 2016 11:50 AM
To: Roper, Matthew D
Cc: Ding, ChiX; Maarten Lankhorst; Adebisi, YetundeX; isg-gms@eclists.intel.com; intel-gfx@lists.freedesktop.org
Subject: Re: [RFC 6/6] drm/i915/vlv: Add intermediate field in intel_crtc_wm_state and handlers for two-level watermark

On Tue, Jun 14, 2016 at 03:57:28PM -0700, Matt Roper wrote:
> On Tue, Jun 14, 2016 at 07:40:54AM -0700, Ding, ChiX wrote:
> > Hi Maarten
> > Thanks for the reply. Please correct me if I'm wrong : 
> > If the intermediate watermark is bigger than any of the old and the new, then it's not really safe.
> > It could be bigger than the FIFO size. 
> > 
> > I did some tests using average value and it ran into FIFO underrun, e.g.
> > [100424.405624] [drm:vlv_invert_wms] Pipe A fifo for primary, 
> > sprite0, sprite 1 : 511 / 0 / 0 [100424.413231] 
> > [drm:vlv_compute_fifo] Pipe A fifo for primary, sprite0, sprite 1 : 
> > 256 / 255 / 0 [100424.413242] [drm:vlv_compute_pipe_wm] Pipe A 
> > [100424.413245] [drm:vlv_compute_pipe_wm] level : 0 [100424.413249] 
> > [drm:vlv_compute_pipe_wm] Watermark values for primary, sprite0, 
> > sprite1 : 136, 135, 0 [100424.413253] 
> > [drm:vlv_compute_intermediate_wm] level : 0 [100424.413257] 
> > [drm:vlv_compute_intermediate_wm] A watermark values for primary, 
> > sprite0, sprite1 : 136, 135, 0 [100424.413261] 
> > [drm:vlv_compute_intermediate_wm] B watermark values for primary, 
> > sprite0, sprite1 : 391, 0, 0 [100424.429823] 
> > [drm:vlv_initial_watermarks] vlv_initial_watermarks
> > 
> > At first, the current active
> > fifo sizes 511 / 0 / 0 for primary, sprite0, sprite 1 watermark 391, 
> > 0, 0
> > 
> > Then the optimal values are calculated : 
> > fifo sizes 256 / 255 / 0
> > watermarks 136, 135, 0
> > 
> > The average watermark for primary plane is 391+136/2 = 263, bigger 
> > than the primary plane fifo size for optimal : 256. It causes 
> > underrun later on when writing intermediate watermark and the fifo 
> > size is changed to 256 for primary plane by vlv_pipe_set_fifo_size() function.
> > 
> > If I use min() for intermediate watermark, I don't run into underrun 
> > issue in the tests.  My understanding is that with min(), in some 
> > cases we get 0,0 for watermark , but it's still a legitimate value.
> > Please let me know if I'm mistaken.
> > 
> > 
> > Another question: the sum of fifo sizes for primary plane, sprite 0, 
> > sprite 1 should always be 512-1 = 511.  But the watermarks of the 
> > planes don't need to add up to 511. They need to stay under or at 
> > most equal to the respective fifo size, right?
> 
> I may be overlooking something since I haven't studied VLV/CHV in as 
> much detail as the other platforms, but there are basically two parts 
> to the per-crtc watermark calculations we do during the atomic 'check'
> phase.
> 
> The first part is slicing up the fifo/DDB between the various planes 
> that are in use.  The fifo buffer is a fixed size resource made up of 
> blocks, but it's programmable how much of that buffer is devoted to 
> driving a specific plane.  The formula we use for slicing up the fifo 
> is to just give each plane a number of blocks proportional to the 
> fraction of the total data rate that the plane contributes (with some 
> extra logic at the end to deal with leftover blocks due to integer division).
> 
> The second part is calculating the watermark values for each plane.  
> The fifo holds data that's been fetched from RAM for the display 
> controller to consume; the watermark values (measured in fifo blocks) 
> are basically telling the hardware how/when it needs to wake up and 
> fetch more data from RAM into the fifo to ensure the fifo doesn't 
> reach empty.  If you set the watermark value for a plane too low, then 
> the hardware won't fetch more data from RAM early enough and the fifo 
> will run dry before new data from RAM shows up; when this happens, the 
> display controller doesn't have any data to tell it how to keep 
> driving the display so you can get various kinds of display corruption 
> (and dmesg warnings about a FIFO underrun).  Programming watermarks 
> too high is always "fine" from a correctness point of view --- you're 
> just causing the hardware to wake up and fetch more data from RAM 
> earlier than it really had to.  You may be impacting power usage, but you're safe from underruns/corruption.
> 
> For intermediate watermarks, we need "worst case" values that satisfy 
> both the old and new configuration (i.e., won't cause underruns for 
> either config).  So I think you want to be using max() instead of 
> min() to calculate your intermediate values.  Once you've calculated 
> the worst-case (max) values for each plane, I figure you'd want to 
> work backwards from there and slice up your intermediate per-place 
> fifo allocation second...I guess you'd probably just set each plane's 
> fifo size equal to its watermark value to start with, then distribute 
> the remaining blocks equally between all of the planes that are active 
> in either new or old configuration (and if you don't have any 
> remaining blocks, then there's no valid intermediate configuration so 
> we should probably fail the request).
> 
> Again, this is just completely off the top of my head and I haven't 
> actually looked at the VLV/CHV bspecs lately, so I might be completely 
> overlooking some important detail about these platforms.

There won't be any intermediate FIFO sizes. The FIFO repartitioning happens atomically with plane updates, or rather should, but the exact nature of the register double buffering is still a bit of a mystery.
How exactly the double buffering works may dictate how we have to do things, so until someone figures it out, I don't think there's any point in trying to fix the watermark updates.

step 1. figure out the DSBARB double buffering step 2. ?
step 3. profit

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

Re: [RFC 6/6] drm/i915/vlv: Add intermediate field in intel_crtc_wm_state and handlers for two-level watermark

[Ville Syrjälä]

On Wed, Jun 15, 2016 at 12:49:49PM +0300, Ville Syrjälä wrote:
> On Tue, Jun 14, 2016 at 03:57:28PM -0700, Matt Roper wrote:
> > On Tue, Jun 14, 2016 at 07:40:54AM -0700, Ding, ChiX wrote:
> > > Hi Maarten
> > > Thanks for the reply. Please correct me if I'm wrong : 
> > > If the intermediate watermark is bigger than any of the old and the new, then it's not really safe.
> > > It could be bigger than the FIFO size. 
> > > 
> > > I did some tests using average value and it ran into FIFO underrun, e.g.
> > > [100424.405624] [drm:vlv_invert_wms] Pipe A fifo for primary, sprite0, sprite 1 : 511 / 0 / 0 
> > > [100424.413231] [drm:vlv_compute_fifo] Pipe A fifo for primary, sprite0, sprite 1 : 256 / 255 / 0
> > > [100424.413242] [drm:vlv_compute_pipe_wm] Pipe A
> > > [100424.413245] [drm:vlv_compute_pipe_wm] level : 0
> > > [100424.413249] [drm:vlv_compute_pipe_wm] Watermark values for primary, sprite0, sprite1 : 136, 135, 0
> > > [100424.413253] [drm:vlv_compute_intermediate_wm] level : 0
> > > [100424.413257] [drm:vlv_compute_intermediate_wm] A watermark values for primary, sprite0, sprite1 : 136, 135, 0
> > > [100424.413261] [drm:vlv_compute_intermediate_wm] B watermark values for primary, sprite0, sprite1 : 391, 0, 0
> > > [100424.429823] [drm:vlv_initial_watermarks] vlv_initial_watermarks
> > > 
> > > At first, the current active
> > > fifo sizes 511 / 0 / 0 for primary, sprite0, sprite 1
> > > watermark 391, 0, 0
> > > 
> > > Then the optimal values are calculated : 
> > > fifo sizes 256 / 255 / 0
> > > watermarks 136, 135, 0
> > > 
> > > The average watermark for primary plane is 391+136/2 = 263, bigger
> > > than the primary plane fifo size for optimal : 256. It causes underrun
> > > later on when writing intermediate watermark and the fifo size is
> > > changed to 256 for primary plane by vlv_pipe_set_fifo_size() function.
> > > 
> > > If I use min() for intermediate watermark, I don't run into underrun
> > > issue in the tests.  My understanding is that with min(), in some
> > > cases we get 0,0 for watermark , but it's still a legitimate value.
> > > Please let me know if I'm mistaken.
> > > 
> > > 
> > > Another question: the sum of fifo sizes for primary plane, sprite 0,
> > > sprite 1 should always be 512-1 = 511.  But the watermarks of the
> > > planes don't need to add up to 511. They need to stay under or at most
> > > equal to the respective fifo size, right?
> > 
> > I may be overlooking something since I haven't studied VLV/CHV in as
> > much detail as the other platforms, but there are basically two parts to
> > the per-crtc watermark calculations we do during the atomic 'check'
> > phase.
> > 
> > The first part is slicing up the fifo/DDB between the various planes
> > that are in use.  The fifo buffer is a fixed size resource made up of
> > blocks, but it's programmable how much of that buffer is devoted to
> > driving a specific plane.  The formula we use for slicing up the fifo is
> > to just give each plane a number of blocks proportional to the fraction
> > of the total data rate that the plane contributes (with some extra logic
> > at the end to deal with leftover blocks due to integer division).
> > 
> > The second part is calculating the watermark values for each plane.  The
> > fifo holds data that's been fetched from RAM for the display controller
> > to consume; the watermark values (measured in fifo blocks) are basically
> > telling the hardware how/when it needs to wake up and fetch more data
> > from RAM into the fifo to ensure the fifo doesn't reach empty.  If you
> > set the watermark value for a plane too low, then the hardware won't
> > fetch more data from RAM early enough and the fifo will run dry before
> > new data from RAM shows up; when this happens, the display controller
> > doesn't have any data to tell it how to keep driving the display so you
> > can get various kinds of display corruption (and dmesg warnings about a
> > FIFO underrun).  Programming watermarks too high is always "fine" from a
> > correctness point of view --- you're just causing the hardware to wake
> > up and fetch more data from RAM earlier than it really had to.  You may
> > be impacting power usage, but you're safe from underruns/corruption.
> > 
> > For intermediate watermarks, we need "worst case" values that satisfy
> > both the old and new configuration (i.e., won't cause underruns for
> > either config).  So I think you want to be using max() instead of min()
> > to calculate your intermediate values.  Once you've calculated the
> > worst-case (max) values for each plane, I figure you'd want to work
> > backwards from there and slice up your intermediate per-place fifo
> > allocation second...I guess you'd probably just set each plane's fifo
> > size equal to its watermark value to start with, then distribute the
> > remaining blocks equally between all of the planes that are active in
> > either new or old configuration (and if you don't have any remaining
> > blocks, then there's no valid intermediate configuration so we should
> > probably fail the request).
> > 
> > Again, this is just completely off the top of my head and I haven't
> > actually looked at the VLV/CHV bspecs lately, so I might be completely
> > overlooking some important detail about these platforms.
> 
> There won't be any intermediate FIFO sizes. The FIFO repartitioning
> happens atomically with plane updates, or rather should, but the exact
> nature of the register double buffering is still a bit of a mystery.
> How exactly the double buffering works may dictate how we have to do
> things, so until someone figures it out, I don't think there's any
> point in trying to fix the watermark updates.
> 
> step 1. figure out the DSBARB double buffering
> step 2. ?
> step 3. profit

Bah, since no one else seems to have bothered to do this, I went ahead
and wrote a small test to exercise the DSPARB double buffering. The
results surprised me in a positive way; The hardware does seem to work
in a sane way, that is, only the DSPARB bits for pipe A appear to be
latched by pipe A vblank, pipe B bits by pipe B vblank etc. I've run
through all combinations of two pipes and didn't see any cross-pipe
latching happening. However I only tested with primary planes thus
far. I think I'll still give it another go with some sprites enabled,
just to be extra sure it's working correctly...

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