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

[Srinivas, Vidya]

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> Sent: Thursday, March 29, 2018 5:03 PM
> To: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: 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
> 
> 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 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.
> > 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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Ville Syrjälä
> Intel OTC