| Message ID | 1522662715-13640-19-git-send-email-vidya.srinivas@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@linux.intel.com> > Signed-off-by: Vidya Srinivas <vidya.srinivas@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. > /* 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.. > + else > + I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id), > + ((crtc_w + 1) << 16)|(crtc_h + 1)); > I915_WRITE_FW(PLANE_POS(pipe, plane_id), 0); > } else { > I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) | crtc_x); > @@ -969,6 +978,9 @@ intel_check_sprite_plane(struct intel_plane *plane, > return -EINVAL; > } > > + if (fb->format->format == DRM_FORMAT_NV12) > + goto check_plane_surface; > + > /* setup can_scale, min_scale, max_scale */ > if (INTEL_GEN(dev_priv) >= 9) { > if (state->base.fb) > @@ -1112,6 +1124,7 @@ intel_check_sprite_plane(struct intel_plane *plane, > dst->y1 = crtc_y; > dst->y2 = crtc_y + crtc_h; > > +check_plane_surface: > if (INTEL_GEN(dev_priv) >= 9) { > ret = skl_check_plane_surface(crtc_state, state); > if (ret)
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@linux.intel.com> > Signed-off-by: Vidya Srinivas <vidya.srinivas@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. > + > /* 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)); > + else > + I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id), > + ((crtc_w + 1) << 16)|(crtc_h + 1)); > I915_WRITE_FW(PLANE_POS(pipe, plane_id), 0); > } else { > I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) | crtc_x); > @@ -969,6 +978,9 @@ intel_check_sprite_plane(struct intel_plane *plane, > return -EINVAL; > } > > + if (fb->format->format == DRM_FORMAT_NV12) > + goto check_plane_surface; > + > /* setup can_scale, min_scale, max_scale */ > if (INTEL_GEN(dev_priv) >= 9) { > if (state->base.fb) > @@ -1112,6 +1124,7 @@ intel_check_sprite_plane(struct intel_plane *plane, > dst->y1 = crtc_y; > dst->y2 = crtc_y + crtc_h; > > +check_plane_surface: > if (INTEL_GEN(dev_priv) >= 9) { > ret = skl_check_plane_surface(crtc_state, state); > if (ret)
> -----Original Message----- > From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] > Sent: Wednesday, April 4, 2018 1:28 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com> > > Signed-off-by: Vidya Srinivas <vidya.srinivas@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. > > + else > > + I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id), > > + ((crtc_w + 1) << 16)|(crtc_h + 1)); > > I915_WRITE_FW(PLANE_POS(pipe, plane_id), 0); > > } else { > > I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) > | crtc_x); > > @@ -969,6 +978,9 @@ intel_check_sprite_plane(struct intel_plane > *plane, > > return -EINVAL; > > } > > > > + if (fb->format->format == DRM_FORMAT_NV12) > > + goto check_plane_surface; > > + > > /* setup can_scale, min_scale, max_scale */ > > if (INTEL_GEN(dev_priv) >= 9) { > > if (state->base.fb) > > @@ -1112,6 +1124,7 @@ intel_check_sprite_plane(struct intel_plane > *plane, > > dst->y1 = crtc_y; > > dst->y2 = crtc_y + crtc_h; > > > > +check_plane_surface: > > if (INTEL_GEN(dev_priv) >= 9) { > > ret = skl_check_plane_surface(crtc_state, state); > > if (ret) >
> -----Original Message----- > From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] > Sent: Wednesday, April 4, 2018 1:33 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com> > > Signed-off-by: Vidya Srinivas <vidya.srinivas@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. > > + > > /* 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)); > > + else > > + I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id), > > + ((crtc_w + 1) << 16)|(crtc_h + 1)); > > I915_WRITE_FW(PLANE_POS(pipe, plane_id), 0); > > } else { > > I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) > | crtc_x); > > @@ -969,6 +978,9 @@ intel_check_sprite_plane(struct intel_plane > *plane, > > return -EINVAL; > > } > > > > + if (fb->format->format == DRM_FORMAT_NV12) > > + goto check_plane_surface; > > + > > /* setup can_scale, min_scale, max_scale */ > > if (INTEL_GEN(dev_priv) >= 9) { > > if (state->base.fb) > > @@ -1112,6 +1124,7 @@ intel_check_sprite_plane(struct intel_plane > *plane, > > dst->y1 = crtc_y; > > dst->y2 = crtc_y + crtc_h; > > > > +check_plane_surface: > > if (INTEL_GEN(dev_priv) >= 9) { > > ret = skl_check_plane_surface(crtc_state, state); > > if (ret) >
Op 04-04-18 om 10:06 schreef Srinivas, Vidya: > >> -----Original Message----- >> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] >> Sent: Wednesday, April 4, 2018 1:33 PM >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- >> gfx@lists.freedesktop.org >> Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com> >>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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. >>> + >>> /* 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)); >>> + else >>> + I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id), >>> + ((crtc_w + 1) << 16)|(crtc_h + 1)); >>> I915_WRITE_FW(PLANE_POS(pipe, plane_id), 0); >>> } else { >>> I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) >> | crtc_x); >>> @@ -969,6 +978,9 @@ intel_check_sprite_plane(struct intel_plane >> *plane, >>> return -EINVAL; >>> } >>> >>> + if (fb->format->format == DRM_FORMAT_NV12) >>> + goto check_plane_surface; >>> + >>> /* setup can_scale, min_scale, max_scale */ >>> if (INTEL_GEN(dev_priv) >= 9) { >>> if (state->base.fb) >>> @@ -1112,6 +1124,7 @@ intel_check_sprite_plane(struct intel_plane >> *plane, >>> dst->y1 = crtc_y; >>> dst->y2 = crtc_y + crtc_h; >>> >>> +check_plane_surface: >>> if (INTEL_GEN(dev_priv) >= 9) { >>> ret = skl_check_plane_surface(crtc_state, state); >>> if (ret)
> -----Original Message----- > From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] > Sent: Wednesday, April 4, 2018 1:40 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com] > >> Sent: Wednesday, April 4, 2018 1:33 PM > >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > >> gfx@lists.freedesktop.org > >> Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com> > >>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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. Sorry, yeah that’s correct. I will remove it only from skl_update_plane. Regarding the crtc_w and crtc_h, I see that when its not even, fifo underruns are seen. Kindly suggest on that. Thank you. > >>> + > >>> /* 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)); > >>> + else > >>> + I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id), > >>> + ((crtc_w + 1) << 16)|(crtc_h + 1)); > >>> I915_WRITE_FW(PLANE_POS(pipe, plane_id), 0); > >>> } else { > >>> I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) > >> | crtc_x); > >>> @@ -969,6 +978,9 @@ intel_check_sprite_plane(struct intel_plane > >> *plane, > >>> return -EINVAL; > >>> } > >>> > >>> + if (fb->format->format == DRM_FORMAT_NV12) > >>> + goto check_plane_surface; > >>> + > >>> /* setup can_scale, min_scale, max_scale */ > >>> if (INTEL_GEN(dev_priv) >= 9) { > >>> if (state->base.fb) > >>> @@ -1112,6 +1124,7 @@ intel_check_sprite_plane(struct intel_plane > >> *plane, > >>> dst->y1 = crtc_y; > >>> dst->y2 = crtc_y + crtc_h; > >>> > >>> +check_plane_surface: > >>> if (INTEL_GEN(dev_priv) >= 9) { > >>> ret = skl_check_plane_surface(crtc_state, state); > >>> if (ret) >
> -----Original Message----- > From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] > Sent: Wednesday, April 4, 2018 1:40 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com] > >> Sent: Wednesday, April 4, 2018 1:33 PM > >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > >> gfx@lists.freedesktop.org > >> Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com> > >>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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. Thanks Vidya > >>> + > >>> /* 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)); > >>> + else > >>> + I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id), > >>> + ((crtc_w + 1) << 16)|(crtc_h + 1)); > >>> I915_WRITE_FW(PLANE_POS(pipe, plane_id), 0); > >>> } else { > >>> I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) > >> | crtc_x); > >>> @@ -969,6 +978,9 @@ intel_check_sprite_plane(struct intel_plane > >> *plane, > >>> return -EINVAL; > >>> } > >>> > >>> + if (fb->format->format == DRM_FORMAT_NV12) > >>> + goto check_plane_surface; > >>> + > >>> /* setup can_scale, min_scale, max_scale */ > >>> if (INTEL_GEN(dev_priv) >= 9) { > >>> if (state->base.fb) > >>> @@ -1112,6 +1124,7 @@ intel_check_sprite_plane(struct intel_plane > >> *plane, > >>> dst->y1 = crtc_y; > >>> dst->y2 = crtc_y + crtc_h; > >>> > >>> +check_plane_surface: > >>> if (INTEL_GEN(dev_priv) >= 9) { > >>> ret = skl_check_plane_surface(crtc_state, state); > >>> if (ret) >
Op 04-04-18 om 10:29 schreef Srinivas, Vidya: > >> -----Original Message----- >> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] >> Sent: Wednesday, April 4, 2018 1:40 PM >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- >> gfx@lists.freedesktop.org >> Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com] >>>> Sent: Wednesday, April 4, 2018 1:33 PM >>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- >>>> gfx@lists.freedesktop.org >>>> Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com> >>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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. :) ~Maarten
> -----Original Message----- > From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] > Sent: Wednesday, April 4, 2018 2:02 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com] > >> Sent: Wednesday, April 4, 2018 1:40 PM > >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > >> gfx@lists.freedesktop.org > >> Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com] > >>>> Sent: Wednesday, April 4, 2018 1:33 PM > >>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > >>>> gfx@lists.freedesktop.org > >>>> Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com> > >>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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
Op 04-04-18 om 10:04 schreef Srinivas, Vidya: > >> -----Original Message----- >> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] >> Sent: Wednesday, April 4, 2018 1:28 PM >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- >> gfx@lists.freedesktop.org >> Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com> >>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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.
> -----Original Message----- > From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] > Sent: Wednesday, April 4, 2018 2:18 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com] > >> Sent: Wednesday, April 4, 2018 1:28 PM > >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > >> gfx@lists.freedesktop.org > >> Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com> > >>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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.
Op 04-04-18 om 10:51 schreef Srinivas, Vidya: > >> -----Original Message----- >> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] >> Sent: Wednesday, April 4, 2018 2:18 PM >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- >> gfx@lists.freedesktop.org >> Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com] >>>> Sent: Wednesday, April 4, 2018 1:28 PM >>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- >>>> gfx@lists.freedesktop.org >>>> Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com> >>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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. ~Maarten
> -----Original Message----- > From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] > Sent: Wednesday, April 4, 2018 2:37 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com] > >> Sent: Wednesday, April 4, 2018 2:18 PM > >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > >> gfx@lists.freedesktop.org > >> Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com] > >>>> Sent: Wednesday, April 4, 2018 1:28 PM > >>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > >>>> gfx@lists.freedesktop.org > >>>> Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com> > >>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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
Op 05-04-18 om 07:18 schreef Srinivas, Vidya: > >> -----Original Message----- >> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] >> Sent: Wednesday, April 4, 2018 2:37 PM >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- >> gfx@lists.freedesktop.org >> Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com] >>>> Sent: Wednesday, April 4, 2018 2:18 PM >>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- >>>> gfx@lists.freedesktop.org >>>> Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com] >>>>>> Sent: Wednesday, April 4, 2018 1:28 PM >>>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- >>>>>> gfx@lists.freedesktop.org >>>>>> Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com> >>>>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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
> -----Original Message----- > From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] > Sent: Thursday, April 5, 2018 3:32 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com] > >> Sent: Wednesday, April 4, 2018 2:37 PM > >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > >> gfx@lists.freedesktop.org > >> Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com] > >>>> Sent: Wednesday, April 4, 2018 2:18 PM > >>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > >>>> gfx@lists.freedesktop.org > >>>> Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com] > >>>>>> Sent: Wednesday, April 4, 2018 1:28 PM > >>>>>> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > >>>>>> gfx@lists.freedesktop.org > >>>>>> Cc: Lankhorst, Maarten <maarten.lankhorst@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@linux.intel.com> > >>>>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@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
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; + } + /* 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)); + else + I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id), + ((crtc_w + 1) << 16)|(crtc_h + 1)); I915_WRITE_FW(PLANE_POS(pipe, plane_id), 0); } else { I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) | crtc_x); @@ -969,6 +978,9 @@ intel_check_sprite_plane(struct intel_plane *plane, return -EINVAL; } + if (fb->format->format == DRM_FORMAT_NV12) + goto check_plane_surface; + /* setup can_scale, min_scale, max_scale */ if (INTEL_GEN(dev_priv) >= 9) { if (state->base.fb) @@ -1112,6 +1124,7 @@ intel_check_sprite_plane(struct intel_plane *plane, dst->y1 = crtc_y; dst->y2 = crtc_y + crtc_h; +check_plane_surface: if (INTEL_GEN(dev_priv) >= 9) { ret = skl_check_plane_surface(crtc_state, state); if (ret)
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@linux.intel.com> Signed-off-by: Vidya Srinivas <vidya.srinivas@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(-)