[Intel-gfx] [PATCH v18 18/18] drm/i915: Keep plane size mult of 4 for NV12

[Srinivas, Vidya]

> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
> Sent: Thursday, March 29, 2018 2:19 PM
> To: Srinivas, Vidya <vidya.srinivas at intel.com>; intel-
> gfx at 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 at linux.intel.com>
> > Signed-off-by: Vidya Srinivas <vidya.srinivas at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h    | 2 ++
> >  drivers/gpu/drm/i915/intel_sprite.c | 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