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

[Maarten Lankhorst]

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.