[Intel-gfx] [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12
Saarinen, Jani
jani.saarinen at intel.com
Tue Apr 17 06:06:17 UTC 2018
+wider-list.
> -----Original Message-----
> From: Srinivas, Vidya
> Sent: tiistai 17. huhtikuuta 2018 7.59
> To: 'Maarten Lankhorst' <maarten.lankhorst at linux.intel.com>; 'intel-gfx-
> trybot at lists.freedesktop.org' <intel-gfx-trybot at lists.freedesktop.org>
> Cc: Kamath, Sunil <sunil.kamath at intel.com>; Saarinen, Jani
> <jani.saarinen at intel.com>
> 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 at linux.intel.com>; intel-gfx-
>
> > trybot at lists.freedesktop.org
>
> > Cc: Kamath, Sunil <sunil.kamath at intel.com>; Saarinen, Jani
>
> > <jani.saarinen at 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.lankhorst at linux.intel.com
> <mailto:maarten.lankhorst at linux.intel.com> ]
>
> > > Sent: Monday, April 16, 2018 8:00 PM
>
> > > To: Srinivas, Vidya <vidya.srinivas at intel.com
> <mailto:vidya.srinivas at intel.com> >; intel-gfx-
>
> > > trybot at lists.freedesktop.org <mailto:trybot at lists.freedesktop.org>
>
> > > Cc: Kamath, Sunil <sunil.kamath at intel.com
> <mailto:sunil.kamath at intel.com> >; Saarinen, Jani
>
> > > <jani.saarinen at intel.com <mailto:jani.saarinen at 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.lankhorst at linux.intel.com
> <mailto:maarten.lankhorst at linux.intel.com> >
>
> > > > Signed-off-by: Vidya Srinivas <vidya.srinivas at intel.com
> <mailto:vidya.srinivas at 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.
>
>
>
> 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;
>
> > > > + }
>
>
More information about the Intel-gfx
mailing list