[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:02 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:29 schreef 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.
> Reject non multiples of 4. Userspace can adapt. :)

:) okay. Thank you. I will update the patch.

Regards
Vidya

> 
> ~Maarten