<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="EN-US" link="#0563C1" vlink="#954F72"> <div class="WordSection1"> <o:p> </o:p> <a name="_MailEndCompose"><o:p> </o:p></a> <a name="_____replyseparator"></a>> -----Original Message----- > From: Srinivas, Vidya > Sent: Tuesday, April 17, 2018 8:17 AM > To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; intel-gfx- > trybot@lists.freedesktop.org > Cc: Kamath, Sunil <sunil.kamath@intel.com>; Saarinen, Jani > <jani.saarinen@intel.com> > Subject: RE: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for > NV12 > > > > > -----Original Message----- > > From: Maarten Lankhorst [<a href="mailto:maarten.lankhorst@linux.intel.com">mailto:maarten.lankhorst@linux.intel.com</a>] > > Sent: Monday, April 16, 2018 8:00 PM > > To: Srinivas, Vidya <<a href="mailto:vidya.srinivas@intel.com">vidya.srinivas@intel.com</a>>; intel-gfx- > > <a href="mailto:trybot@lists.freedesktop.org">trybot@lists.freedesktop.org</a> > > Cc: Kamath, Sunil <<a href="mailto:sunil.kamath@intel.com">sunil.kamath@intel.com</a>>; Saarinen, Jani > > <<a href="mailto:jani.saarinen@intel.com">jani.saarinen@intel.com</a>> > > 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 <<a href="mailto:maarten.lankhorst@linux.intel.com">maarten.lankhorst@linux.intel.com</a>> > > > Signed-off-by: Vidya Srinivas <<a href="mailto:vidya.srinivas@intel.com">vidya.srinivas@intel.com</a>> > > > --- > > > 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. <o:p> </o:p> Example:<o:p></o:p> During kms_plane_scaling-with-rotation execution - print cooridinates here: src->x1, src->x2, src->y1, src->y2<o:p></o:p> <o:p> </o:p> [ 43.542044] [drm:intel_check_sprite_plane] *ERROR* Before adjust: 1040: 0 16 0 16<o:p></o:p> [ 43.542054] [drm:intel_check_sprite_plane] *ERROR* After adjust: 1054: 0 15 0 15<o:p></o:p> [ 43.542058] [drm:intel_check_sprite_plane] *ERROR* Before check plane surface: 1135: 0 15 0 15<o:p></o:p> [ 43.570827] [drm:intel_cpu_fifo_underrun_irq_handler] *ERROR* CPU pipe A FIFO underrun<o:p></o:p> <o:p> </o:p> 16x16 becomes 15x15 after rect adjustment in check_sprite_plane<o:p></o:p> <o:p> </o:p> During kms_plane_scaling-with-clipping-clamping – print coordinates here src->x1, src->x2, src->y1, src->y2<o:p></o:p> <o:p> </o:p> [ 125.432029] [drm:intel_check_primary_plane] *ERROR* Before check plane state: 12998: 0 300 0 300<o:p></o:p> [ 125.432041] [drm:intel_check_primary_plane] *ERROR* After check plane state: 13004: 0 257 0 159<o:p></o:p> [ 128.057081] [drm:intel_cpu_fifo_underrun_irq_handler] *ERROR* CPU pipe A FIFO underrun<o:p></o:p> <o:p> </o:p> 300x300 after primary plane clipping becomes 257 in check_primary_plane<o:p></o:p> <o:p> </o:p> All these generate fifo underruns. In the first example here, 16x16, before adjustment we check if it is multiplier<o:p></o:p> of 4, and since it is, we allow. But after rect_adjust it becomes 15x15<o:p></o:p> In example 2, clipping clamping, 300x300 is the value before clipping in primary plane, since it is a multiple of 4, we allow<o:p></o:p> But after clipping it becomes 257x159.<o:p></o:p> <o:p> </o:p> <o:p> </o:p> > > > > > 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. <o:p> </o:p> This part is required only if we retain 16x16 buffer, Because after adjust_size it becomes<o:p></o:p> 15x15 as I have mentioned above. Then our minimum criteria for NV12 fails and our igt buffer<o:p></o:p> As of now is 16x16. To get a IGT pass I had added this work around.<o:p></o:p> <o:p> </o:p> > > :) > > > + 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; > > > + } <o:p> </o:p> </div> </body> </html>