| Message ID | 1522310762-5055-19-git-send-email-vidya.srinivas@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 29-03-18 om 10:06 schreef Vidya Srinivas: > As per display WA 1106, to avoid corruption issues > NV12 plane height needs to be multiplier of 4 > Hence we modify the fb src and destination height > and width to be multiples of 4. Without this, pipe > fifo underruns were seen on APL and KBL. > > 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 | 8 ++++++++ > 2 files changed, 10 insertions(+) > > 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 538d938..9f466c6 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -261,6 +261,14 @@ skl_update_plane(struct intel_plane *plane, > crtc_w--; > crtc_h--; > > + if (fb->format->format == DRM_FORMAT_NV12) { > + src_w = MULT4(src_w); > + src_h = MULT4(src_h); > + crtc_w = MULT4(crtc_w); > + crtc_h = MULT4(crtc_h); > + DRM_ERROR("%d %d %d %d\n", src_w, src_h, crtc_w, crtc_h); > + } > + > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) Nearly there! Do we have limitations for width too? But I think we shouldn't ever adjust src for any format. This means that we should probably get rid of the drm_rect_adjust_size call in intel_check_sprite_plane. If any limitations of NV12 are hit, we should reject with -EINVAL instead so userspace can decide what to do. The best place to put those checks is probably in skl_update_scaler, where they will be checked by the primary plane too. This will mean the tests fail, but that can be fixed by selecting 16 as width/height for NV12 in IGT. If you change it to 16 you can put my r-b on it. Also I think we should put the same limitations for width and height being a multiple in intel_framebuffer_init. And on a final note for patch ordering, put the workaround and gen10 patch before enabling nv12 support. ~Maarten
> -----Original Message----- > From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] > Sent: Thursday, March 29, 2018 2:19 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH v18 18/18] drm/i915: Keep plane size mult of > 4 for NV12 > > Op 29-03-18 om 10:06 schreef Vidya Srinivas: > > As per display WA 1106, to avoid corruption issues > > NV12 plane height needs to be multiplier of 4 Hence we modify the fb > > src and destination height and width to be multiples of 4. Without > > this, pipe fifo underruns were seen on APL and KBL. > > > > 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 | 8 ++++++++ > > 2 files changed, 10 insertions(+) > > > > 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 538d938..9f466c6 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -261,6 +261,14 @@ skl_update_plane(struct intel_plane *plane, > > crtc_w--; > > crtc_h--; > > > > + if (fb->format->format == DRM_FORMAT_NV12) { > > + src_w = MULT4(src_w); > > + src_h = MULT4(src_h); > > + crtc_w = MULT4(crtc_w); > > + crtc_h = MULT4(crtc_h); > > + DRM_ERROR("%d %d %d %d\n", src_w, src_h, crtc_w, > crtc_h); > > + } > > + > > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > > Nearly there! > > Do we have limitations for width too? But I think we shouldn't ever adjust > src for any format. > This means that we should probably get rid of the drm_rect_adjust_size call > in intel_check_sprite_plane. > > If any limitations of NV12 are hit, we should reject with -EINVAL instead so > userspace can decide what to do. > The best place to put those checks is probably in skl_update_scaler, where > they will be checked by the primary plane too. > > This will mean the tests fail, but that can be fixed by selecting 16 as > width/height for NV12 in IGT. If you change it to 16 you can put my r-b on it. > > Also I think we should put the same limitations for width and height being a > multiple in intel_framebuffer_init. > > And on a final note for patch ordering, put the workaround and gen10 patch > before enabling nv12 support. Thank you. Okay, I will make these changes and check once. The limitation in Framebuffer init is already present where it expects width and height >= 16 As per bspec no minimum for width has been mentioned. And regarding the Add check for primary plane (same like sprite), can we add that as a separate patch Because if we add it with NV12 series, it would be like adding the changes and Returning before executing them. Right now range check only exists for NV12 in skl_update_scaler. My worry was: If the width and height are not multiplier of 4 do we return from skl_update_scaler? What if some other user level program wants to set src width and height 23x23 etc? I will check if we remove the src aligning from skl_update_plane and just keep the Destination as multiplier of 4 in skl_update_plane. Regarding the reordering, I will make the change and float the series. Thank you So much for all the support and pointers. If no fifo underruns are seen with just keeping the dest width and height mult of 4, We anyways don’t do the drm_rect_adjust_size, then we can avoid putting any Limitation (other than range check) in skl_update_scaler correct? Regards Vidya > > ~Maarten
On Thu, Mar 29, 2018 at 01:36:02PM +0530, Vidya Srinivas wrote: > As per display WA 1106, to avoid corruption issues > NV12 plane height needs to be multiplier of 4 > Hence we modify the fb src and destination height > and width to be multiples of 4. Without this, pipe > fifo underruns were seen on APL and KBL. > > 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 | 8 ++++++++ > 2 files changed, 10 insertions(+) > > 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 538d938..9f466c6 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -261,6 +261,14 @@ skl_update_plane(struct intel_plane *plane, > crtc_w--; > crtc_h--; > > + if (fb->format->format == DRM_FORMAT_NV12) { > + src_w = MULT4(src_w); > + src_h = MULT4(src_h); > + crtc_w = MULT4(crtc_w); > + crtc_h = MULT4(crtc_h); No macros like this pls. I want to know what it's doing. Also this is wrong. You can't increase src_w/h without potentially pushing the scale factor past the hardware limits. > + DRM_ERROR("%d %d %d %d\n", src_w, src_h, crtc_w, crtc_h); No user triggrable errors. Also this doesn't even explain what it's printing. > + } > + > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > Sent: Thursday, March 29, 2018 2:56 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten > <maarten.lankhorst@intel.com> > Subject: Re: [Intel-gfx] [PATCH v18 18/18] drm/i915: Keep plane size mult of > 4 for NV12 > > On Thu, Mar 29, 2018 at 01:36:02PM +0530, Vidya Srinivas wrote: > > As per display WA 1106, to avoid corruption issues > > NV12 plane height needs to be multiplier of 4 Hence we modify the fb > > src and destination height and width to be multiples of 4. Without > > this, pipe fifo underruns were seen on APL and KBL. > > > > 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 | 8 ++++++++ > > 2 files changed, 10 insertions(+) > > > > 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 538d938..9f466c6 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -261,6 +261,14 @@ skl_update_plane(struct intel_plane *plane, > > crtc_w--; > > crtc_h--; > > > > + if (fb->format->format == DRM_FORMAT_NV12) { > > + src_w = MULT4(src_w); > > + src_h = MULT4(src_h); > > + crtc_w = MULT4(crtc_w); > > + crtc_h = MULT4(crtc_h); > > No macros like this pls. I want to know what it's doing. Also this is wrong. > You can't increase src_w/h without potentially pushing the scale factor past > the hardware limits. Thank you. I am trying with not modifying the src w and h. Instead we just Avoid the truncation (drm_rect_adjust_size and remaining things) for NV12 in Intel_check_sprite_plane. I will keep only crtc_w and crtc_h as a mult of 4 and See if no fifo underruns are seen. Regards Vidya > > > + DRM_ERROR("%d %d %d %d\n", src_w, src_h, crtc_w, > crtc_h); > > No user triggrable errors. Also this doesn't even explain what it's printing. Sorry about this. This went in by mistake. Will remove it. > > > + } > > + > > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > > -- > > 2.7.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC
On Thu, Mar 29, 2018 at 09:29:06AM +0000, Srinivas, Vidya wrote: > > > > -----Original Message----- > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > > Sent: Thursday, March 29, 2018 2:56 PM > > To: Srinivas, Vidya <vidya.srinivas@intel.com> > > Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten > > <maarten.lankhorst@intel.com> > > Subject: Re: [Intel-gfx] [PATCH v18 18/18] drm/i915: Keep plane size mult of > > 4 for NV12 > > > > On Thu, Mar 29, 2018 at 01:36:02PM +0530, Vidya Srinivas wrote: > > > As per display WA 1106, to avoid corruption issues > > > NV12 plane height needs to be multiplier of 4 Hence we modify the fb > > > src and destination height and width to be multiples of 4. Without > > > this, pipe fifo underruns were seen on APL and KBL. > > > > > > 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 | 8 ++++++++ > > > 2 files changed, 10 insertions(+) > > > > > > 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 538d938..9f466c6 100644 > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > @@ -261,6 +261,14 @@ skl_update_plane(struct intel_plane *plane, > > > crtc_w--; > > > crtc_h--; > > > > > > + if (fb->format->format == DRM_FORMAT_NV12) { > > > + src_w = MULT4(src_w); > > > + src_h = MULT4(src_h); > > > + crtc_w = MULT4(crtc_w); > > > + crtc_h = MULT4(crtc_h); > > > > No macros like this pls. I want to know what it's doing. Also this is wrong. > > You can't increase src_w/h without potentially pushing the scale factor past > > the hardware limits. > > > Thank you. I am trying with not modifying the src w and h. Instead we just > Avoid the truncation (drm_rect_adjust_size and remaining things) for NV12 in > Intel_check_sprite_plane. I will keep only crtc_w and crtc_h as a mult of 4 and > See if no fifo underruns are seen. The limitations are on the scaler source side, so I doubt that will do anything. So I don't even understand why we're playing around with the destination coordinates here. Anywyay, I thought we already agreed to just return an error when things are misaligned? > > Regards > Vidya > > > > > > + DRM_ERROR("%d %d %d %d\n", src_w, src_h, crtc_w, > > crtc_h); > > > > No user triggrable errors. Also this doesn't even explain what it's printing. > Sorry about this. This went in by mistake. Will remove it. > > > > > + } > > > + > > > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > > > > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > > > -- > > > 2.7.4 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel OTC
> -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > Sent: Thursday, March 29, 2018 3:04 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten > <maarten.lankhorst@intel.com> > Subject: Re: [Intel-gfx] [PATCH v18 18/18] drm/i915: Keep plane size mult of > 4 for NV12 > > On Thu, Mar 29, 2018 at 09:29:06AM +0000, Srinivas, Vidya wrote: > > > > > > > -----Original Message----- > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > > > Sent: Thursday, March 29, 2018 2:56 PM > > > To: Srinivas, Vidya <vidya.srinivas@intel.com> > > > Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten > > > <maarten.lankhorst@intel.com> > > > Subject: Re: [Intel-gfx] [PATCH v18 18/18] drm/i915: Keep plane size > > > mult of > > > 4 for NV12 > > > > > > On Thu, Mar 29, 2018 at 01:36:02PM +0530, Vidya Srinivas wrote: > > > > As per display WA 1106, to avoid corruption issues > > > > NV12 plane height needs to be multiplier of 4 Hence we modify the > > > > fb src and destination height and width to be multiples of 4. > > > > Without this, pipe fifo underruns were seen on APL and KBL. > > > > > > > > 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 | 8 ++++++++ > > > > 2 files changed, 10 insertions(+) > > > > > > > > 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 538d938..9f466c6 100644 > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > > @@ -261,6 +261,14 @@ skl_update_plane(struct intel_plane *plane, > > > > crtc_w--; > > > > crtc_h--; > > > > > > > > + if (fb->format->format == DRM_FORMAT_NV12) { > > > > + src_w = MULT4(src_w); > > > > + src_h = MULT4(src_h); > > > > + crtc_w = MULT4(crtc_w); > > > > + crtc_h = MULT4(crtc_h); > > > > > > No macros like this pls. I want to know what it's doing. Also this is > wrong. > > > You can't increase src_w/h without potentially pushing the scale > > > factor past the hardware limits. > > > > > > Thank you. I am trying with not modifying the src w and h. Instead we > > just Avoid the truncation (drm_rect_adjust_size and remaining things) > > for NV12 in Intel_check_sprite_plane. I will keep only crtc_w and > > crtc_h as a mult of 4 and See if no fifo underruns are seen. > > The limitations are on the scaler source side, so I doubt that will do > anything. So I don't even understand why we're playing around with the > destination coordinates here. > > Anywyay, I thought we already agreed to just return an error when things > are misaligned? The limitation for height is on scaler side. But for Gen9, GLK and GLV There is a workaround 1106 which says: Display corruption/color shift observed when using NV12 with 270 rotation or 90 rotation + horizontal flip. WA: NV12 with 270 rotation or 90 rotation + horizontal flip requires the programmed plane height to be a multiple of 4. Based on all the trials we have done, if we don't keep the dest width and height aligned to mult of 4, we see fifo underrun on APL and KBL. > > > > > Regards > > Vidya > > > > > > > > > + DRM_ERROR("%d %d %d %d\n", src_w, src_h, crtc_w, > > > crtc_h); > > > > > > No user triggrable errors. Also this doesn't even explain what it's > printing. > > Sorry about this. This went in by mistake. Will remove it. > > > > > > > + } > > > > + > > > > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > > > > > > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > > > > -- > > > > 2.7.4 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > -- > > > Ville Syrjälä > > > Intel OTC > > -- > Ville Syrjälä > Intel OTC
Op 29-03-18 om 11:19 schreef Srinivas, Vidya: > >> -----Original Message----- >> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] >> Sent: Thursday, March 29, 2018 2:19 PM >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- >> gfx@lists.freedesktop.org >> Subject: Re: [Intel-gfx] [PATCH v18 18/18] drm/i915: Keep plane size mult of >> 4 for NV12 >> >> Op 29-03-18 om 10:06 schreef Vidya Srinivas: >>> As per display WA 1106, to avoid corruption issues >>> NV12 plane height needs to be multiplier of 4 Hence we modify the fb >>> src and destination height and width to be multiples of 4. Without >>> this, pipe fifo underruns were seen on APL and KBL. >>> >>> 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 | 8 ++++++++ >>> 2 files changed, 10 insertions(+) >>> >>> 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 538d938..9f466c6 100644 >>> --- a/drivers/gpu/drm/i915/intel_sprite.c >>> +++ b/drivers/gpu/drm/i915/intel_sprite.c >>> @@ -261,6 +261,14 @@ skl_update_plane(struct intel_plane *plane, >>> crtc_w--; >>> crtc_h--; >>> >>> + if (fb->format->format == DRM_FORMAT_NV12) { >>> + src_w = MULT4(src_w); >>> + src_h = MULT4(src_h); >>> + crtc_w = MULT4(crtc_w); >>> + crtc_h = MULT4(crtc_h); >>> + DRM_ERROR("%d %d %d %d\n", src_w, src_h, crtc_w, >> crtc_h); >>> + } >>> + >>> spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); >>> >>> if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) >> Nearly there! >> >> Do we have limitations for width too? But I think we shouldn't ever adjust >> src for any format. >> This means that we should probably get rid of the drm_rect_adjust_size call >> in intel_check_sprite_plane. >> >> If any limitations of NV12 are hit, we should reject with -EINVAL instead so >> userspace can decide what to do. >> The best place to put those checks is probably in skl_update_scaler, where >> they will be checked by the primary plane too. >> >> This will mean the tests fail, but that can be fixed by selecting 16 as >> width/height for NV12 in IGT. If you change it to 16 you can put my r-b on it. >> >> Also I think we should put the same limitations for width and height being a >> multiple in intel_framebuffer_init. >> >> And on a final note for patch ordering, put the workaround and gen10 patch >> before enabling nv12 support. > Thank you. Okay, I will make these changes and check once. The limitation in > Framebuffer init is already present where it expects width and height >= 16 > As per bspec no minimum for width has been mentioned. And regarding the > Add check for primary plane (same like sprite), can we add that as a separate patch > Because if we add it with NV12 series, it would be like adding the changes and > Returning before executing them. I don't think we'll lose much if we fail to create a fb that's not a multiple of 4 in height and width. Since the NV12 format is defined in terms of 4x4 pixel blocks, I don't think it would be a loss to fail to create it, if we can't even display it. > Right now range check only exists for NV12 in skl_update_scaler. My worry was: > If the width and height are not multiplier of 4 do we return from skl_update_scaler? We should always refuse to show when the src height is not a multiple of 4, and return -EINVAL. > What if some other user level program wants to set src width and height 23x23 etc? Then userspace will see that it will fail with -EINVAL, if it's done by a compositor with a TEST_ONLY commit, it will see the src cannot be set and either choose a different size or fallback to software rendering before displaying the output. This is still better than silently succeeding, but showing something different. > I will check if we remove the src aligning from skl_update_plane and just keep the > Destination as multiplier of 4 in skl_update_plane. I think it's more likely the src that needs to be a multiple of 4. I don't think there's much of a failure in destination. > Regarding the reordering, I will make the change and float the series. Thank you > So much for all the support and pointers. > > If no fifo underruns are seen with just keeping the dest width and height mult of 4, > We anyways don’t do the drm_rect_adjust_size, then we can avoid putting any > Limitation (other than range check) in skl_update_scaler correct? Perhaps, but wouldn't hurt to be paranoid.
> -----Original Message----- > From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] > Sent: Thursday, March 29, 2018 3:59 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH v18 18/18] drm/i915: Keep plane size mult of > 4 for NV12 > > Op 29-03-18 om 11:19 schreef Srinivas, Vidya: > > > >> -----Original Message----- > >> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] > >> Sent: Thursday, March 29, 2018 2:19 PM > >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > >> gfx@lists.freedesktop.org > >> Subject: Re: [Intel-gfx] [PATCH v18 18/18] drm/i915: Keep plane size > >> mult of > >> 4 for NV12 > >> > >> Op 29-03-18 om 10:06 schreef Vidya Srinivas: > >>> As per display WA 1106, to avoid corruption issues > >>> NV12 plane height needs to be multiplier of 4 Hence we modify the fb > >>> src and destination height and width to be multiples of 4. Without > >>> this, pipe fifo underruns were seen on APL and KBL. > >>> > >>> 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 | 8 ++++++++ > >>> 2 files changed, 10 insertions(+) > >>> > >>> 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 538d938..9f466c6 100644 > >>> --- a/drivers/gpu/drm/i915/intel_sprite.c > >>> +++ b/drivers/gpu/drm/i915/intel_sprite.c > >>> @@ -261,6 +261,14 @@ skl_update_plane(struct intel_plane *plane, > >>> crtc_w--; > >>> crtc_h--; > >>> > >>> + if (fb->format->format == DRM_FORMAT_NV12) { > >>> + src_w = MULT4(src_w); > >>> + src_h = MULT4(src_h); > >>> + crtc_w = MULT4(crtc_w); > >>> + crtc_h = MULT4(crtc_h); > >>> + DRM_ERROR("%d %d %d %d\n", src_w, src_h, crtc_w, > >> crtc_h); > >>> + } > >>> + > >>> spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > >>> > >>> if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > >> Nearly there! > >> > >> Do we have limitations for width too? But I think we shouldn't ever > >> adjust src for any format. > >> This means that we should probably get rid of the > >> drm_rect_adjust_size call in intel_check_sprite_plane. > >> > >> If any limitations of NV12 are hit, we should reject with -EINVAL > >> instead so userspace can decide what to do. > >> The best place to put those checks is probably in skl_update_scaler, > >> where they will be checked by the primary plane too. > >> > >> This will mean the tests fail, but that can be fixed by selecting 16 > >> as width/height for NV12 in IGT. If you change it to 16 you can put my r-b > on it. > >> > >> Also I think we should put the same limitations for width and height > >> being a multiple in intel_framebuffer_init. > >> > >> And on a final note for patch ordering, put the workaround and gen10 > >> patch before enabling nv12 support. > > Thank you. Okay, I will make these changes and check once. The > > limitation in Framebuffer init is already present where it expects > > width and height >= 16 As per bspec no minimum for width has been > > mentioned. And regarding the Add check for primary plane (same like > > sprite), can we add that as a separate patch Because if we add it with > > NV12 series, it would be like adding the changes and Returning before > executing them. > I don't think we'll lose much if we fail to create a fb that's not a multiple of > 4 in height and width. Since the NV12 format is defined in terms of 4x4 pixel > blocks, I don't think it would be a loss to fail to create it, if we can't even > display it. > > Right now range check only exists for NV12 in skl_update_scaler. My worry > was: > > If the width and height are not multiplier of 4 do we return from > skl_update_scaler? > We should always refuse to show when the src height is not a multiple of 4, > and return -EINVAL. > > What if some other user level program wants to set src width and height > 23x23 etc? > Then userspace will see that it will fail with -EINVAL, if it's done by a > compositor with a TEST_ONLY commit, it will see the src cannot be set and > either choose a different size or fallback to software rendering before > displaying the output. > > This is still better than silently succeeding, but showing something different. Sure, thank you. I will make the changes and float again. > > I will check if we remove the src aligning from skl_update_plane and > > just keep the Destination as multiplier of 4 in skl_update_plane. > I think it's more likely the src that needs to be a multiple of 4. I don't think > there's much of a failure in destination. > > Regarding the reordering, I will make the change and float the series. > > Thank you So much for all the support and pointers. > > > > If no fifo underruns are seen with just keeping the dest width and > > height mult of 4, We anyways don’t do the drm_rect_adjust_size, then > > we can avoid putting any Limitation (other than range check) in > skl_update_scaler correct? > Perhaps, but wouldn't hurt to be paranoid. True, I will add the limitations here as well. Thank you. Regards Vidya
On Thu, Mar 29, 2018 at 12:28:48PM +0200, Maarten Lankhorst wrote: > Op 29-03-18 om 11:19 schreef Srinivas, Vidya: > > > >> -----Original Message----- > >> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] > >> Sent: Thursday, March 29, 2018 2:19 PM > >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > >> gfx@lists.freedesktop.org > >> Subject: Re: [Intel-gfx] [PATCH v18 18/18] drm/i915: Keep plane size mult of > >> 4 for NV12 > >> > >> Op 29-03-18 om 10:06 schreef Vidya Srinivas: > >>> As per display WA 1106, to avoid corruption issues > >>> NV12 plane height needs to be multiplier of 4 Hence we modify the fb > >>> src and destination height and width to be multiples of 4. Without > >>> this, pipe fifo underruns were seen on APL and KBL. > >>> > >>> 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 | 8 ++++++++ > >>> 2 files changed, 10 insertions(+) > >>> > >>> 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 538d938..9f466c6 100644 > >>> --- a/drivers/gpu/drm/i915/intel_sprite.c > >>> +++ b/drivers/gpu/drm/i915/intel_sprite.c > >>> @@ -261,6 +261,14 @@ skl_update_plane(struct intel_plane *plane, > >>> crtc_w--; > >>> crtc_h--; > >>> > >>> + if (fb->format->format == DRM_FORMAT_NV12) { > >>> + src_w = MULT4(src_w); > >>> + src_h = MULT4(src_h); > >>> + crtc_w = MULT4(crtc_w); > >>> + crtc_h = MULT4(crtc_h); > >>> + DRM_ERROR("%d %d %d %d\n", src_w, src_h, crtc_w, > >> crtc_h); > >>> + } > >>> + > >>> spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > >>> > >>> if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > >> Nearly there! > >> > >> Do we have limitations for width too? But I think we shouldn't ever adjust > >> src for any format. > >> This means that we should probably get rid of the drm_rect_adjust_size call > >> in intel_check_sprite_plane. > >> > >> If any limitations of NV12 are hit, we should reject with -EINVAL instead so > >> userspace can decide what to do. > >> The best place to put those checks is probably in skl_update_scaler, where > >> they will be checked by the primary plane too. > >> > >> This will mean the tests fail, but that can be fixed by selecting 16 as > >> width/height for NV12 in IGT. If you change it to 16 you can put my r-b on it. > >> > >> Also I think we should put the same limitations for width and height being a > >> multiple in intel_framebuffer_init. > >> > >> And on a final note for patch ordering, put the workaround and gen10 patch > >> before enabling nv12 support. > > Thank you. Okay, I will make these changes and check once. The limitation in > > Framebuffer init is already present where it expects width and height >= 16 > > As per bspec no minimum for width has been mentioned. And regarding the > > Add check for primary plane (same like sprite), can we add that as a separate patch > > Because if we add it with NV12 series, it would be like adding the changes and > > Returning before executing them. > I don't think we'll lose much if we fail to create a fb that's not a multiple of 4 in > height and width. Since the NV12 format is defined in terms of 4x4 pixel blocks, > I don't think it would be a loss to fail to create it, if we can't even display it. The fb size is pretty much irrelevant since you can scan out just part of it anyway. Anyway, as far as the src rect adjustments for sprites go, I guess we can just switch SKL sprites over to the primary plane codepath and add the relevant checks there. Hmm, and it looks like the primary plane packed YUV stuff is already pretty much broken since we don't check for odd widths there. Anyway I just hacked together this: git://github.com/vsyrjala/linux.git plane_check_skl It's sittin on top of https://patchwork.freedesktop.org/series/39390/, which itself could use some review... > > Right now range check only exists for NV12 in skl_update_scaler. My worry was: > > If the width and height are not multiplier of 4 do we return from skl_update_scaler? > We should always refuse to show when the src height is not a multiple of 4, and return -EINVAL. > > What if some other user level program wants to set src width and height 23x23 etc? > Then userspace will see that it will fail with -EINVAL, if it's done by a compositor with a TEST_ONLY commit, > it will see the src cannot be set and either choose a different size or fallback to software rendering before > displaying the output. > > This is still better than silently succeeding, but showing something different. > > I will check if we remove the src aligning from skl_update_plane and just keep the > > Destination as multiplier of 4 in skl_update_plane. > I think it's more likely the src that needs to be a multiple of 4. I don't think there's > much of a failure in destination. > > Regarding the reordering, I will make the change and float the series. Thank you > > So much for all the support and pointers. > > > > If no fifo underruns are seen with just keeping the dest width and height mult of 4, > > We anyways don’t do the drm_rect_adjust_size, then we can avoid putting any > > Limitation (other than range check) in skl_update_scaler correct? > Perhaps, but wouldn't hurt to be paranoid. > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > Sent: Thursday, March 29, 2018 5:03 PM > To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH v18 18/18] drm/i915: Keep plane size mult of > 4 for NV12 > > On Thu, Mar 29, 2018 at 12:28:48PM +0200, Maarten Lankhorst wrote: > > Op 29-03-18 om 11:19 schreef Srinivas, Vidya: > > > > > >> -----Original Message----- > > >> From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] > > >> Sent: Thursday, March 29, 2018 2:19 PM > > >> To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > > >> gfx@lists.freedesktop.org > > >> Subject: Re: [Intel-gfx] [PATCH v18 18/18] drm/i915: Keep plane > > >> size mult of > > >> 4 for NV12 > > >> > > >> Op 29-03-18 om 10:06 schreef Vidya Srinivas: > > >>> As per display WA 1106, to avoid corruption issues > > >>> NV12 plane height needs to be multiplier of 4 Hence we modify the > > >>> fb src and destination height and width to be multiples of 4. > > >>> Without this, pipe fifo underruns were seen on APL and KBL. > > >>> > > >>> 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 | 8 ++++++++ > > >>> 2 files changed, 10 insertions(+) > > >>> > > >>> 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 538d938..9f466c6 100644 > > >>> --- a/drivers/gpu/drm/i915/intel_sprite.c > > >>> +++ b/drivers/gpu/drm/i915/intel_sprite.c > > >>> @@ -261,6 +261,14 @@ skl_update_plane(struct intel_plane *plane, > > >>> crtc_w--; > > >>> crtc_h--; > > >>> > > >>> + if (fb->format->format == DRM_FORMAT_NV12) { > > >>> + src_w = MULT4(src_w); > > >>> + src_h = MULT4(src_h); > > >>> + crtc_w = MULT4(crtc_w); > > >>> + crtc_h = MULT4(crtc_h); > > >>> + DRM_ERROR("%d %d %d %d\n", src_w, src_h, crtc_w, > > >> crtc_h); > > >>> + } > > >>> + > > >>> spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > >>> > > >>> if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > > >> Nearly there! > > >> > > >> Do we have limitations for width too? But I think we shouldn't ever > > >> adjust src for any format. > > >> This means that we should probably get rid of the > > >> drm_rect_adjust_size call in intel_check_sprite_plane. > > >> > > >> If any limitations of NV12 are hit, we should reject with -EINVAL > > >> instead so userspace can decide what to do. > > >> The best place to put those checks is probably in > > >> skl_update_scaler, where they will be checked by the primary plane too. > > >> > > >> This will mean the tests fail, but that can be fixed by selecting > > >> 16 as width/height for NV12 in IGT. If you change it to 16 you can put > my r-b on it. > > >> > > >> Also I think we should put the same limitations for width and > > >> height being a multiple in intel_framebuffer_init. > > >> > > >> And on a final note for patch ordering, put the workaround and > > >> gen10 patch before enabling nv12 support. > > > Thank you. Okay, I will make these changes and check once. The > > > limitation in Framebuffer init is already present where it expects > > > width and height >= 16 As per bspec no minimum for width has been > > > mentioned. And regarding the Add check for primary plane (same like > > > sprite), can we add that as a separate patch Because if we add it > > > with NV12 series, it would be like adding the changes and Returning > before executing them. > > I don't think we'll lose much if we fail to create a fb that's not a > > multiple of 4 in height and width. Since the NV12 format is defined in > > terms of 4x4 pixel blocks, I don't think it would be a loss to fail to create it, > if we can't even display it. > > The fb size is pretty much irrelevant since you can scan out just part of it > anyway. > > Anyway, as far as the src rect adjustments for sprites go, I guess we can just > switch SKL sprites over to the primary plane codepath and add the relevant > checks there. Hmm, and it looks like the primary plane packed YUV stuff is > already pretty much broken since we don't check for odd widths there. > > Anyway I just hacked together this: > git://github.com/vsyrjala/linux.git plane_check_skl > > It's sittin on top of https://patchwork.freedesktop.org/series/39390/, > which itself could use some review... Thank you. Went through the code series and git. For now, I just added the change https://patchwork.freedesktop.org/patch/214227/ where we just skip the truncation of sprite, and right from framebuffer_init I added the restriction that src width and height needs to be multiplier of 4. In skl_update_plane, only the destination will be aligned to multiplier of 4. Regards Vidya > > > > Right now range check only exists for NV12 in skl_update_scaler. My > worry was: > > > If the width and height are not multiplier of 4 do we return from > skl_update_scaler? > > We should always refuse to show when the src height is not a multiple of > 4, and return -EINVAL. > > > What if some other user level program wants to set src width and height > 23x23 etc? > > Then userspace will see that it will fail with -EINVAL, if it's done > > by a compositor with a TEST_ONLY commit, it will see the src cannot be > > set and either choose a different size or fallback to software rendering > before displaying the output. > > > > This is still better than silently succeeding, but showing something > different. > > > I will check if we remove the src aligning from skl_update_plane and > > > just keep the Destination as multiplier of 4 in skl_update_plane. > > I think it's more likely the src that needs to be a multiple of 4. I > > don't think there's much of a failure in destination. > > > Regarding the reordering, I will make the change and float the > > > series. Thank you So much for all the support and pointers. > > > > > > If no fifo underruns are seen with just keeping the dest width and > > > height mult of 4, We anyways don’t do the drm_rect_adjust_size, then > > > we can avoid putting any Limitation (other than range check) in > skl_update_scaler correct? > > Perhaps, but wouldn't hurt to be paranoid. > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC
> -----Original Message----- > From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com] > Sent: Thursday, March 29, 2018 2:19 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH v18 18/18] drm/i915: Keep plane size mult of > 4 for NV12 > > Op 29-03-18 om 10:06 schreef Vidya Srinivas: > > As per display WA 1106, to avoid corruption issues > > NV12 plane height needs to be multiplier of 4 Hence we modify the fb > > src and destination height and width to be multiples of 4. Without > > this, pipe fifo underruns were seen on APL and KBL. > > > > 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 | 8 ++++++++ > > 2 files changed, 10 insertions(+) > > > > 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 538d938..9f466c6 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -261,6 +261,14 @@ skl_update_plane(struct intel_plane *plane, > > crtc_w--; > > crtc_h--; > > > > + if (fb->format->format == DRM_FORMAT_NV12) { > > + src_w = MULT4(src_w); > > + src_h = MULT4(src_h); > > + crtc_w = MULT4(crtc_w); > > + crtc_h = MULT4(crtc_h); > > + DRM_ERROR("%d %d %d %d\n", src_w, src_h, crtc_w, > crtc_h); > > + } > > + > > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > > Nearly there! > > Do we have limitations for width too? But I think we shouldn't ever adjust > src for any format. > This means that we should probably get rid of the drm_rect_adjust_size call > in intel_check_sprite_plane. > > If any limitations of NV12 are hit, we should reject with -EINVAL instead so > userspace can decide what to do. > The best place to put those checks is probably in skl_update_scaler, where > they will be checked by the primary plane too. > > This will mean the tests fail, but that can be fixed by selecting 16 as > width/height for NV12 in IGT. If you change it to 16 you can put my r-b on it. > > Also I think we should put the same limitations for width and height being a > multiple in intel_framebuffer_init. > > And on a final note for patch ordering, put the workaround and gen10 patch > before enabling nv12 support. Thank you. I have added the restriction in intel_framebuffer_init and have re-ordered the series. Have also floated the i-g-t patch with 16x16 buffer and I have included your r-b. Kindly have a check. Currently since we have 17x17 the restriction hits and kernel message fb dimensions are not right is seen for tests. If the 16x16 i-g-t patch gets merged, we can get the results. On my side, I have tested with many buffers (mult of 4) and no underruns are seen. https://patchwork.freedesktop.org/series/39670/ (rev 7) > > ~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 538d938..9f466c6 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -261,6 +261,14 @@ skl_update_plane(struct intel_plane *plane, crtc_w--; crtc_h--; + if (fb->format->format == DRM_FORMAT_NV12) { + src_w = MULT4(src_w); + src_h = MULT4(src_h); + crtc_w = MULT4(crtc_w); + crtc_h = MULT4(crtc_h); + DRM_ERROR("%d %d %d %d\n", src_w, src_h, crtc_w, crtc_h); + } + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
As per display WA 1106, to avoid corruption issues NV12 plane height needs to be multiplier of 4 Hence we modify the fb src and destination height and width to be multiples of 4. Without this, pipe fifo underruns were seen on APL and KBL. 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 | 8 ++++++++ 2 files changed, 10 insertions(+)