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

Thu Mar 29 11:33:12 UTC 2018

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

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