+wider-list. > -----Original Message----- > From: Srinivas, Vidya > Sent: tiistai 17. huhtikuuta 2018 7.59 > To: 'Maarten Lankhorst' <maarten.lankhorst@xxxxxxxxxxxxxxx>; 'intel-gfx- > trybot@xxxxxxxxxxxxxxxxxxxxx' <intel-gfx-trybot@xxxxxxxxxxxxxxxxxxxxx> > Cc: Kamath, Sunil <sunil.kamath@xxxxxxxxx>; Saarinen, Jani > <jani.saarinen@xxxxxxxxx> > Subject: RE: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12 > > > > > > > -----Original Message----- > > > From: Srinivas, Vidya > > > Sent: Tuesday, April 17, 2018 8:17 AM > > > To: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>; intel-gfx- > > > trybot@xxxxxxxxxxxxxxxxxxxxx > > > Cc: Kamath, Sunil <sunil.kamath@xxxxxxxxx>; Saarinen, Jani > > > <jani.saarinen@xxxxxxxxx> > > > Subject: RE: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for > > > NV12 > > > > > > > > > > > > > -----Original Message----- > > > > From: Maarten Lankhorst [mailto:maarten.lankhorst@xxxxxxxxxxxxxxx > <mailto:maarten.lankhorst@xxxxxxxxxxxxxxx> ] > > > > Sent: Monday, April 16, 2018 8:00 PM > > > > To: Srinivas, Vidya <vidya.srinivas@xxxxxxxxx > <mailto:vidya.srinivas@xxxxxxxxx> >; intel-gfx- > > > > trybot@xxxxxxxxxxxxxxxxxxxxx <mailto:trybot@xxxxxxxxxxxxxxxxxxxxx> > > > > Cc: Kamath, Sunil <sunil.kamath@xxxxxxxxx > <mailto:sunil.kamath@xxxxxxxxx> >; Saarinen, Jani > > > > <jani.saarinen@xxxxxxxxx <mailto:jani.saarinen@xxxxxxxxx> > > > > > 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.lankhorst@xxxxxxxxxxxxxxx > <mailto:maarten.lankhorst@xxxxxxxxxxxxxxx> > > > > > > Signed-off-by: Vidya Srinivas <vidya.srinivas@xxxxxxxxx > <mailto:vidya.srinivas@xxxxxxxxx> > > > > > > --- > > > > > 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. > > > > Example: > > During kms_plane_scaling-with-rotation execution - print cooridinates here: > src->x1, src->x2, src->y1, src->y2 > > > > [ 43.542044] [drm:intel_check_sprite_plane] *ERROR* Before adjust: 1040: > 0 16 0 16 > > [ 43.542054] [drm:intel_check_sprite_plane] *ERROR* After adjust: 1054: 0 > 15 0 15 > > [ 43.542058] [drm:intel_check_sprite_plane] *ERROR* Before check plane > surface: 1135: 0 15 0 15 > > [ 43.570827] [drm:intel_cpu_fifo_underrun_irq_handler] *ERROR* CPU > pipe A FIFO underrun > > > > 16x16 becomes 15x15 after rect adjustment in check_sprite_plane > > > > During kms_plane_scaling-with-clipping-clamping – print coordinates here > src->x1, src->x2, src->y1, src->y2 > > > > [ 125.432029] [drm:intel_check_primary_plane] *ERROR* Before check > plane state: 12998: 0 300 0 300 > > [ 125.432041] [drm:intel_check_primary_plane] *ERROR* After check plane > state: 13004: 0 257 0 159 > > [ 128.057081] [drm:intel_cpu_fifo_underrun_irq_handler] *ERROR* CPU > pipe A FIFO underrun > > > > 300x300 after primary plane clipping becomes 257 in check_primary_plane > > > > All these generate fifo underruns. In the first example here, 16x16, before > adjustment we check if it is multiplier > > of 4, and since it is, we allow. But after rect_adjust it becomes 15x15 > > In example 2, clipping clamping, 300x300 is the value before clipping in > primary plane, since it is a multiple of 4, we allow > > But after clipping it becomes 257x159. > > > > > > > > > > > > > > > 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. > > > > This part is required only if we retain 16x16 buffer, Because after adjust_size > it becomes > > 15x15 as I have mentioned above. Then our minimum criteria for NV12 fails > and our igt buffer > > As of now is 16x16. To get a IGT pass I had added this work around. > > > > > > :) > > > > > + 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
