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

Thu Apr 5 10:37:44 UTC 2018

> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
> Sent: Thursday, April 5, 2018 3:32 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 05-04-18 om 07:18 schreef 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.
> I've done testing and had very reliable underruns with height not a multiple
> of 2, as could be seen in the earlier kms_nv12 series. Clipping/clamping
> tests should be fixed at least not to get into such a bad situation for NV12,
> and take alignment into account.

Sure, will address the same. Thank you.

Regards
Vidya

> 
> ~Maarten