Re: [PATCH 13/14] drm/i915: Workaround VLV/CHV sprite1->sprite0 enable underrun

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 12-12-16 om 21:35 schreef ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> On VLV/CHV enabling sprite0 when sprite1 has already been enabled may
> lead to an underrun. This only happens when sprite0 FIFO size is zero
> prior to enabling it. Hence an effective workaround is to always
> allocate at least one cacheline for sprite0 when sprite1 is active.
>
> I've not observed this sort of failure during any other type of plane
> enable/disable sequence.
>
> Testcase: igt/kms_plane_blinker
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index fa882cdcac52..75a5bde43723 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1006,6 +1006,12 @@ static uint16_t vlv_compute_wm_level(const struct intel_crtc_state *crtc_state,
>  	return min_t(int, wm, USHRT_MAX);
>  }
>  
> +static bool vlv_need_sprite0_fifo_workaround(unsigned int active_planes)
> +{
> +	return (active_planes & (BIT(PLANE_SPRITE0) |
> +				 BIT(PLANE_SPRITE1))) == BIT(PLANE_SPRITE1);
> +}
> +
>  static int vlv_compute_fifo(struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> @@ -1016,12 +1022,25 @@ static int vlv_compute_fifo(struct intel_crtc_state *crtc_state)
>  	int num_active_planes = hweight32(active_planes);
>  	const int fifo_size = 511;
>  	int fifo_extra, fifo_left = fifo_size;
> +	int sprite0_fifo_extra = 0;
>  	unsigned int total_rate;
>  	enum plane_id plane_id;
>  
> +	/*
> +	 * When enabling sprite0 after sprite1 has already been enabled
> +	 * we tend to get an underrun unless sprite0 already has some
> +	 * FIFO space allcoated. Hence we always allocate at least one
> +	 * cacheline for sprite0 whenever sprite1 is enabled.
> +	 *
> +	 * All other plane enable sequences appear immune to this problem.
> +	 */
> +	if (vlv_need_sprite0_fifo_workaround(active_planes))
> +		sprite0_fifo_extra = 1;
I don't think you need crtc_state->active_planes just for this, it adds a lot of tracking for something that could be done by

if (noninverted->plane[SPRITE1] && !noninverted->plane[SPRITE0])
/* allocate 1 for sprite 0 */

Maybe drop that patch?
>  	total_rate = noninverted->plane[PLANE_PRIMARY] +
>  		noninverted->plane[PLANE_SPRITE0] +
> -		noninverted->plane[PLANE_SPRITE1];
> +		noninverted->plane[PLANE_SPRITE1] +
> +		sprite0_fifo_extra;
>  
>  	if (total_rate > fifo_size)
>  		return -EINVAL;
> @@ -1042,6 +1061,9 @@ static int vlv_compute_fifo(struct intel_crtc_state *crtc_state)
>  		fifo_left -= fifo_state->plane[plane_id];
>  	}
>  
> +	fifo_state->plane[PLANE_SPRITE0] += sprite0_fifo_extra;
> +	fifo_left -= sprite0_fifo_extra;
> +
>  	fifo_state->plane[PLANE_CURSOR] = 63;
>  
>  	fifo_extra = DIV_ROUND_UP(fifo_left, num_active_planes ?: 1);


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