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

Thu Apr 5 10:02:09 UTC 2018

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.

~Maarten