[Intel-gfx] [PATCH v19 18/18] drm/i915: Set src size restrictions for NV12

[Srinivas, Vidya]

> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
> Sent: Wednesday, April 4, 2018 1:40 PM
> To: Srinivas, Vidya <vidya.srinivas at intel.com>; intel-
> gfx at lists.freedesktop.org
> Cc: Lankhorst, Maarten <maarten.lankhorst at intel.com>
> Subject: Re: [Intel-gfx] [PATCH v19 18/18] drm/i915: Set src size restrictions
> for NV12
> 
> Op 04-04-18 om 10:06 schreef Srinivas, Vidya:
> >
> >> -----Original Message-----
> >> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
> >> Sent: Wednesday, April 4, 2018 1:33 PM
> >> To: Srinivas, Vidya <vidya.srinivas at intel.com>; intel-
> >> gfx at lists.freedesktop.org
> >> Cc: Lankhorst, Maarten <maarten.lankhorst at intel.com>
> >> Subject: Re: [Intel-gfx] [PATCH v19 18/18] drm/i915: Set src size
> >> restrictions for NV12
> >>
> >> 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 at linux.intel.com>
> >>> Signed-off-by: Vidya Srinivas <vidya.srinivas at 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;
> >>> +	}
> >> Also on a side note, don't add DRM_DEBUG_KMS in update_plane or
> >> disable_plane calls. It can take an arbitrary amount of time,
> >> resulting in an atomic update failure.
> > Sure, thank you. These are anyway duplicate checks. I will remove them.
> > The main one is done in intel_framebuffer_init as suggested by you.
> They're not duplicate, they're done for related reasons:
> 1. It doesn't make sense to allow creation of a framebuffer with an invalid
> width/height if the full width/height cannot be used.
> 2. The checks in skl_update_scaler are the ones that will reject even showing
> a subset with invalid width, which can cause the fifo underruns.

Sorry, yeah that’s correct. I will remove it only from skl_update_plane.
Regarding the crtc_w and crtc_h, I see that when its not even,
fifo underruns are seen. Kindly suggest on that. Thank you.

> >>> +
> >>>  	/* 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));
> >>> +		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)
>