> -----Original Message----- > From: Maarten Lankhorst [mailto:maarten.lankhorst@xxxxxxxxxxxxxxx] > Sent: Thursday, April 12, 2018 4:41 PM > To: Srinivas, Vidya <vidya.srinivas@xxxxxxxxx>; Intel Graphics Development > <intel-gfx@xxxxxxxxxxxxxxxxxxxxx>; Ville Syrjälä > <ville.syrjala@xxxxxxxxxxxxxxx> > Cc: Kamath, Sunil <sunil.kamath@xxxxxxxxx>; Saarinen, Jani > <jani.saarinen@xxxxxxxxx> > Subject: Re: [PATCH v1 5/6] drm/i915: Do not do fb src adjustments for > NV12 > > Op 12-04-18 om 12:07 schreef Srinivas, Vidya: > > > >> -----Original Message----- > >> From: Maarten Lankhorst [mailto:maarten.lankhorst@xxxxxxxxxxxxxxx] > >> Sent: Wednesday, April 11, 2018 4:08 PM > >> To: Srinivas, Vidya <vidya.srinivas@xxxxxxxxx>; intel-gfx- > >> trybot@xxxxxxxxxxxxxxxxxxxxx > >> Cc: Kamath, Sunil <sunil.kamath@xxxxxxxxx>; Saarinen, Jani > >> <jani.saarinen@xxxxxxxxx> > >> Subject: Re: [PATCH v1 5/6] drm/i915: Do not do fb src adjustments > >> for > >> NV12 > >> > >> Op 11-04-18 om 11:09 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. > >>> > >>> Credits-to: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > >>> Signed-off-by: Vidya Srinivas <vidya.srinivas@xxxxxxxxx> > >>> --- > >>> drivers/gpu/drm/i915/intel_display.c | 88 > >>> ++++++++++++++++++++++++++++++++++-- > >>> drivers/gpu/drm/i915/intel_sprite.c | 10 +++- > >>> 2 files changed, 92 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/i915/intel_display.c > >>> b/drivers/gpu/drm/i915/intel_display.c > >>> index ebb3f8e..e4cf7a6 100644 > >>> --- a/drivers/gpu/drm/i915/intel_display.c > >>> +++ b/drivers/gpu/drm/i915/intel_display.c > >>> @@ -12951,6 +12951,86 @@ skl_max_scale(struct intel_crtc > >>> *intel_crtc, } > >>> > >>> static int > >>> +intel_primary_plane_state(struct drm_plane_state *plane_state, > >>> + const struct drm_crtc_state *crtc_state, > >>> + int min_scale, int max_scale, > >>> + bool can_position, bool can_update_disabled) { > >>> + struct drm_framebuffer *fb = plane_state->fb; > >>> + struct drm_rect *src = &plane_state->src; > >>> + struct drm_rect *dst = &plane_state->dst; > >>> + unsigned int rotation = plane_state->rotation; > >>> + struct drm_rect clip = {}; > >>> + int hscale, vscale; > >>> + > >>> + WARN_ON(plane_state->crtc && plane_state->crtc != crtc_state- > >>> crtc); > >>> + > >>> + *src = drm_plane_state_src(plane_state); > >>> + *dst = drm_plane_state_dest(plane_state); > >>> + > >>> + if (!fb) { > >>> + plane_state->visible = false; > >>> + return 0; > >>> + } > >>> + > >>> + /* crtc should only be NULL when disabling (i.e., !fb) */ > >>> + if (WARN_ON(!plane_state->crtc)) { > >>> + plane_state->visible = false; > >>> + return 0; > >>> + } > >>> + > >>> + if (!crtc_state->enable && !can_update_disabled) { > >>> + DRM_DEBUG_KMS("Cannot update plane of a disabled > >> CRTC.\n"); > >>> + return -EINVAL; > >>> + } > >>> + > >>> + drm_rect_rotate(src, fb->width << 16, fb->height << 16, rotation); > >>> + > >>> + /* Check scaling */ > >>> + hscale = drm_rect_calc_hscale(src, dst, min_scale, max_scale); > >>> + vscale = drm_rect_calc_vscale(src, dst, min_scale, max_scale); > >>> + if (hscale < 0 || vscale < 0) { > >>> + DRM_DEBUG_KMS("Invalid scaling of plane\n"); > >>> + drm_rect_debug_print("src: ", &plane_state->src, true); > >>> + drm_rect_debug_print("dst: ", &plane_state->dst, false); > >>> + return -ERANGE; > >>> + } > >>> + > >>> + if (crtc_state->enable) > >>> + drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2, > >> &clip.y2); > >>> + > >>> + if (fb->format->format == DRM_FORMAT_NV12) { > >>> + plane_state->visible = true; > >>> + goto skip_clip; > >>> + } > >>> + > >>> + plane_state->visible = > >>> + drm_rect_clip_scaled(src, dst, &clip, hscale, vscale); > >> The real problem is that it needs to be a multiple of 4. I think the > >> clipping here is harmless, we should just adjust intel_check_sprite_plane > for >= SKL. > >> This will make it so we don't have to duplicate its checks. > >> > >> For NV12 we still want to call clip_rect_scaled, but then adjust all > >> coordinates by dividing by 4 on before the check, and multiplying > >> with 4 after? > >> > > Thank you. Have made the changes in > > https://patchwork.freedesktop.org/patch/216682/ > > With this, I did not see any underruns. For now, have a WA only when > > we pass 16x16 Because, with that it further clips down and gets > > rejected in skl_update_scaler as it is Less than 16. If we increase our > buffer in igt, then this issue wont be there. > > Please have a check. Initially, I tried the /4 before the adjustments and *4 > later, that wouldn’t work > > We need to have the 16.16 values multiplier of 4. So, just put it towards > the end of both plane checks. > Well, this is annoying. > > It seems we never rejected subpixel precision before, and resorted to > clipping instead of rejecting. > Could we be more strict about this without breaking existing userspace? > Hi, I am sorry. Not sure if I understood this clearly. The new data (fb info) association is done in check_primary_plane and check_sprite_plane just before the clipping/adjust. So, could not understand where before in the code, we could reject. With your suggestion of /4 and *4 implemented in https://patchwork.freedesktop.org/patch/216867/ All tests passed on both pipes on my APL without any underruns. Even IGT BAT resulted in PASS. Can we not use this approach? > ~Maarten _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
