[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 2:37 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:51 schreef Srinivas, Vidya:
> >
> >> -----Original Message-----
> >> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
> >> Sent: Wednesday, April 4, 2018 2:18 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:04 schreef Srinivas, Vidya:
> >>>> -----Original Message-----
> >>>> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
> >>>> Sent: Wednesday, April 4, 2018 1:28 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;
> >>>>> +	}
> >>>>> +
> >>>> 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.
> >>> Have done it right from intel_framebuffer_init onwards, have done it
> >>> in skl_update_scaler also In fact it will get rejected in
> >>> framebuffer init and
> >> all these are duplicate checks, can remove them.
> >>>>>  	/* 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..
> >>> When I align it to mult of 4, 1919 (which was 1920-1) becomes 1920
> back.
> >>> If we don’t do this, I see fifo underruns. The destination width and
> >>> height If not mult of 4, I am seeing underruns.
> >> What you see as 1920 is 1921, which should be underrunning even more
> >> and is out of FB bounds.
> > Sorry, I gave a wrong example here. It doesn’t happen when we scale it to
> full resolution.
> > It happened during other testing when the scaled dest width or height
> > was not multiple of 4.
> >
> 
> We could reject it, but odd that crtc w/h matters. I didn't expect that. :)
> 
> Rejecting with -EINVAL is the best way to go, along with documentation in
> the form of tests that we handle this case correctly.

Hi Maarten,

Completely understand and agree. With these fifo underruns, I have tried 
too many tests and encountered those fifo underruns under diff set of
experiments. I remember seeing them when src width and height were not
x4. When I kept src dimensions x4, I hit underruns during destination testing. But right now, I tried
to reproduce the same - it isn’t happening on my APL. So, I have removed
all restrictions from the series https://patchwork.freedesktop.org/series/38919/ 
(sent to trybot only for now). I have just retained the src height to be min 16 which
Is mentioned in bspec. Now since we have i-g-t 16x16 merged and the only change for
NV12 majorly for underruns is we skip the truncations that were happening in intel_check_sprite
We will get CI results shortly. Due to too many experiments done at my end, I overloaded myself with data
and get confused - when I hit which issue. Apologies for the same.
PS: I removed the src restrictions too  in this series. Because during clipping clamping tests, the fb src
widthxheight generated is like 257x159. This gets rejected from KMD as its not x4.

Thanks
Vidya

> 
> ~Maarten