Re: [PATCH v19 18/18] drm/i915: Set src size restrictions for NV12

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

Op 02-04-18 om 11:51 schreef Vidya Srinivas:
> As per display WA 1106, to avoid corruption issues
> NV12 plane height needs to be multiplier of 4
> We expect the src dimensions to be multiplier of 4
> We fail the case where src width or height is not
> multiple of 4 for NV12.
> We also set the scaler destination height
> and width to be multiple of 4. Without this, pipe
> fifo underruns were seen on APL and KBL. We also
> skip src trunction/adjustments for NV12 case and handle
> the sizes directly in skl_update_plane
>
> Credits-to: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h    |  2 ++
>  drivers/gpu/drm/i915/intel_sprite.c | 19 ++++++++++++++++---
>  2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9c58da0..a1f718d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -159,6 +159,8 @@
>  #define INTEL_I2C_BUS_DVO 1
>  #define INTEL_I2C_BUS_SDVO 2
>  
> +#define MULT4(x) ((x + 3) & ~0x03)
> +
>  /* these are outputs from the chip - integrated only
>     external chips are via DVO or SDVO output */
>  enum intel_output_type {
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index d5dad44..b2292dd 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -255,6 +255,12 @@ skl_update_plane(struct intel_plane *plane,
>  	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
>  	unsigned long irqflags;
>  
> +	if (fb->format->format == DRM_FORMAT_NV12 &&
> +		((src_h % 4) != 0 || (src_w % 4) != 0)) {
> +		DRM_DEBUG_KMS("NV12: src dimensions not valid\n");
> +		return;
> +	}
> +
You can't do this check in skl_update_plane, it's way too late. It should be rejected in the plane check with -EINVAL, or perhaps in
skl_update_scaler.
>  	/* Sizes are 0 based */
>  	src_w--;
>  	src_h--;
> @@ -292,9 +298,12 @@ skl_update_plane(struct intel_plane *plane,
>  			      PS_SCALER_EN | PS_PLANE_SEL(plane_id) | scaler->mode);
>  		I915_WRITE_FW(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
>  		I915_WRITE_FW(SKL_PS_WIN_POS(pipe, scaler_id), (crtc_x << 16) | crtc_y);
> -		I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id),
> -			      ((crtc_w + 1) << 16)|(crtc_h + 1));
> -
> +		if (fb->format->format == DRM_FORMAT_NV12)
> +			I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id),
> +				      (MULT4(crtc_w) << 16) | MULT4(crtc_h));
See the comment above, sizes are 0 based. This will add 1 to the size, so the size is always 1 more than requested.
I don't think it would pass plane CRC tests..
> +		else
> +			I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id),
> +				      ((crtc_w + 1) << 16)|(crtc_h + 1));
>  		I915_WRITE_FW(PLANE_POS(pipe, plane_id), 0);
>  	} else {
>  		I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) | crtc_x);
> @@ -969,6 +978,9 @@ intel_check_sprite_plane(struct intel_plane *plane,
>  		return -EINVAL;
>  	}
>  
> +	if (fb->format->format == DRM_FORMAT_NV12)
> +		goto check_plane_surface;
> +
>  	/* setup can_scale, min_scale, max_scale */
>  	if (INTEL_GEN(dev_priv) >= 9) {
>  		if (state->base.fb)
> @@ -1112,6 +1124,7 @@ intel_check_sprite_plane(struct intel_plane *plane,
>  	dst->y1 = crtc_y;
>  	dst->y2 = crtc_y + crtc_h;
>  
> +check_plane_surface:
>  	if (INTEL_GEN(dev_priv) >= 9) {
>  		ret = skl_check_plane_surface(crtc_state, state);
>  		if (ret)


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