> -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] > Sent: Thursday, March 29, 2018 5:03 PM > To: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > Cc: Srinivas, Vidya <vidya.srinivas@xxxxxxxxx>; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [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@xxxxxxxxxxxxxxx] > > >> Sent: Thursday, March 29, 2018 2:19 PM > > >> To: Srinivas, Vidya <vidya.srinivas@xxxxxxxxx>; intel- > > >> gfx@xxxxxxxxxxxxxxxxxxxxx > > >> Subject: Re: [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@xxxxxxxxxxxxxxx> > > >>> Signed-off-by: Vidya Srinivas <vidya.srinivas@xxxxxxxxx> > > >>> --- > > >>> 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@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
