[PATCH v2 6/6] drm/i915: Do not do fb src adjustments for NV12

[Saarinen, Jani]

Lets move this to intel-gfx to get more eyes on it. 

> -----Original Message-----
> From: Srinivas, Vidya
> Sent: tiistai 17. huhtikuuta 2018 5.47
> 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]
> > Sent: Monday, April 16, 2018 8:00 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 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>
> > > Signed-off-by: Vidya Srinivas <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.
> 
> >
> > 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.
> > :)
> > > +			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;
> > > +		}