[v18,18/18] drm/i915: Keep plane size mult of 4 for NV12
diff mbox

Message ID 1522310762-5055-19-git-send-email-vidya.srinivas@intel.com
State New
Headers show

Commit Message

vsrini4 March 29, 2018, 8:06 a.m. UTC
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(+)

Comments

Maarten Lankhorst March 29, 2018, 8:48 a.m. UTC | #1
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
vsrini4 March 29, 2018, 9:19 a.m. UTC | #2
> -----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
Ville Syrjala March 29, 2018, 9:25 a.m. UTC | #3
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
vsrini4 March 29, 2018, 9:29 a.m. UTC | #4
> -----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
Ville Syrjala March 29, 2018, 9:33 a.m. UTC | #5
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
vsrini4 March 29, 2018, 9:39 a.m. UTC | #6
> -----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
Maarten Lankhorst March 29, 2018, 10:28 a.m. UTC | #7
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.
vsrini4 March 29, 2018, 10:31 a.m. UTC | #8
> -----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
Ville Syrjala March 29, 2018, 11:33 a.m. UTC | #9
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
vsrini4 April 2, 2018, 9:17 a.m. UTC | #10
> -----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
vsrini4 April 2, 2018, 9:55 a.m. UTC | #11
> -----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

Patch
diff mbox

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))