[Intel-gfx] [PATCH v1 5/6] drm/i915: Do not do fb src adjustments for NV12

[Srinivas, Vidya]

> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
> Sent: Thursday, April 12, 2018 4:41 PM
> To: Srinivas, Vidya <vidya.srinivas at intel.com>; Intel Graphics Development
> <intel-gfx at lists.freedesktop.org>; Ville Syrjälä
> <ville.syrjala at linux.intel.com>
> Cc: Kamath, Sunil <sunil.kamath at intel.com>; Saarinen, Jani
> <jani.saarinen at intel.com>
> 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 at linux.intel.com]
> >> Sent: Wednesday, April 11, 2018 4:08 PM
> >> To: Srinivas, Vidya <vidya.srinivas at 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 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 at linux.intel.com>
> >>> Signed-off-by: Vidya Srinivas <vidya.srinivas at intel.com>
> >>> ---
> >>>  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