Lets move this to intel-gfx to get more eyes on it. > -----Original Message----- > From: Srinivas, Vidya > Sent: tiistai 17. huhtikuuta 2018 5.47 > To: Maarten Lankhorst <maarten.lankho...@linux.intel.com>; intel-gfx- > try...@lists.freedesktop.org > Cc: Kamath, Sunil <sunil.kam...@intel.com>; Saarinen, Jani > <jani.saari...@intel.com> > Subject: RE: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12 > > > > > -----Original Message----- > > From: Maarten Lankhorst [mailto:maarten.lankho...@linux.intel.com] > > Sent: Monday, April 16, 2018 8:00 PM > > To: Srinivas, Vidya <vidya.srini...@intel.com>; intel-gfx- > > try...@lists.freedesktop.org > > Cc: Kamath, Sunil <sunil.kam...@intel.com>; Saarinen, Jani > > <jani.saari...@intel.com> > > Subject: Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for > > NV12 > > > > Hmm, more thought about src adjustments... > > > > Op 13-04-18 om 07:22 schreef Vidya Srinivas: > > > We skip src trunction/adjustments for > > > NV12 case and handle the sizes directly. > > > Without this, pipe fifo underruns are seen on APL/KBL. > > > > > > v2: For NV12, making the src coordinates multiplier of 4 > > > > > > Credits-to: Maarten Lankhorst <maarten.lankho...@linux.intel.com> > > > Signed-off-by: Vidya Srinivas <vidya.srini...@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 11 +++++++++++ > > > drivers/gpu/drm/i915/intel_sprite.c | 15 +++++++++++++++ > > > 2 files changed, 26 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index bc83f10..f64708f 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -12978,6 +12978,10 @@ intel_check_primary_plane(struct > > intel_plane *plane, > > > bool can_position = false; > > > int ret; > > > uint32_t pixel_format = 0; > > > + struct drm_plane_state *plane_state = &state->base; > > > + struct drm_rect *src = &plane_state->src; > > > + > > > + *src = drm_plane_state_src(plane_state); > > > > > > if (INTEL_GEN(dev_priv) >= 9) { > > > /* use scaler when colorkey is not required */ @@ -13001,6 > > > +13005,13 @@ intel_check_primary_plane(struct intel_plane *plane, > > > if (!state->base.fb) > > > return 0; > > > > > > + if (pixel_format == DRM_FORMAT_NV12) { > > > + src->x1 = (((src->x1 >> 16)/4)*4) << 16; > > > + src->x2 = (((src->x2 >> 16)/4)*4) << 16; > > > + src->y1 = (((src->y1 >> 16)/4)*4) << 16; > > > + src->y2 = (((src->y2 >> 16)/4)*4) << 16; > > > + } > > > > Lets reject non multiple of 4 coordinates for plane_state's src_x and > > src_y, and before adjustment also reject non multiple of 4 > > src_w/src_h.fter that we can also reject clipping to the right/bottom > > edge of the screen, if the pipe_src_w/h is not a multiple of 4. That > > way an application trying to go beyond the screen. > > kms_plane_scaling and some other tests during execution generate lots of > non mult of 4 buffer width/height. All these tests will show fail in the IGT > BAT. > In some cases, even thought the width and height will be multiple of 4 > before the Adjustment (say our 16x16 buffer), this after adjustment > becomes a 15 in intel_check_sprite_plane. > This again would cause underrun. If we reject non multiple of 4s in our > skl_update_scaler also, then even simple tests with 16x16 like rotation will > fail due to it getting adjusted. > > > > > skl_check_plane_surface could do all the fixups and is allowed to > > return an error code, so we could put all SKL specific NV12 handling in > there. > > It doesn't matter that we calculate potentially illegal values, if we > > never program them and return -EINVAL there. If it turns out we've > > been too paranoid we could relax the condition. > > > > This would mean unified handling for NV12 for primary and sprite. :) > > > > > if (INTEL_GEN(dev_priv) >= 9) { > > > ret = skl_check_plane_surface(crtc_state, state); > > > if (ret) > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > > b/drivers/gpu/drm/i915/intel_sprite.c > > > index 8b7947d..c1dd85e 100644 > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > @@ -1035,11 +1035,20 @@ intel_check_sprite_plane(struct intel_plane > > *plane, > > > return vscale; > > > } > > > > > > + if (fb->format->format == DRM_FORMAT_NV12) { > > > + if (src->x2 >> 16 == 16) > > > + src->x2 = 16 << 16; > > > + if (src->y2 >> 16 == 16) > > > + src->y2 = 16 << 16; > > This part doesn't look required, the magic below for the nv12 format > > is similar.. Just conditionally call adjust_size for format != NV12. > > :) > > > + goto nv12_min_no_clip; > > > + } > > > + > > > /* Make the source viewport size an exact multiple of the > > scaling factors. */ > > > drm_rect_adjust_size(src, > > > drm_rect_width(dst) * hscale - > > drm_rect_width(src), > > > drm_rect_height(dst) * vscale - > > drm_rect_height(src)); > > > > > > +nv12_min_no_clip: > > > drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, > > > state->base.rotation); > > > > > > @@ -1105,6 +1114,12 @@ intel_check_sprite_plane(struct intel_plane > > *plane, > > > src->x2 = (src_x + src_w) << 16; > > > src->y1 = src_y << 16; > > > src->y2 = (src_y + src_h) << 16; > > > + if (fb->format->format == DRM_FORMAT_NV12) { > > > + src->x1 = (((src->x1 >> 16)/4)*4) << 16; > > > + src->x2 = (((src->x2 >> 16)/4)*4) << 16; > > > + src->y1 = (((src->y1 >> 16)/4)*4) << 16; > > > + src->y2 = (((src->y2 >> 16)/4)*4) << 16; > > > + }
_______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
