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

If we don’t keep dest width and height as mult of 4, fifo underruns are seen.
Shall we reject those also? Then it will be safe. As per my observation,
We have only two options. Either change it to mult of 4 or reject it :(
Please suggest.

Thanks
Vidya

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