Re: [PATCH v7 3/6] drm/i915: Add skl_check_nv12_surface for NV12

From: "Srinivas, Vidya" <vidya.srinivas@intel.com>
To: 'Maarten Lankhorst' <maarten.lankhorst@linux.intel.com>,
	"'intel-gfx@lists.freedesktop.org'"
	<intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH v7 3/6] drm/i915: Add skl_check_nv12_surface for NV12
Date: Tue, 8 May 2018 11:37:32 +0000	[thread overview]
Message-ID: <F653A0A18852B74D88578FA2EB7094EAB6881CE0@BGSMSX108.gar.corp.intel.com> (raw)
In-Reply-To: <F653A0A18852B74D88578FA2EB7094EAB6881837@BGSMSX108.gar.corp.intel.com>

> -----Original Message-----
> From: Srinivas, Vidya
> Sent: Tuesday, May 8, 2018 8:36 AM
> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; intel-
> gfx@lists.freedesktop.org
> Subject: RE: [PATCH v7 3/6] drm/i915: Add skl_check_nv12_surface for NV12
> 
> 
> 
> > -----Original Message-----
> > From: Maarten Lankhorst [mailto:maarten.lankhorst@linux.intel.com]
> > Sent: Monday, May 7, 2018 2:56 PM
> > To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-
> > gfx@lists.freedesktop.org
> > Subject: Re: [PATCH v7 3/6] drm/i915: Add skl_check_nv12_surface for
> > NV12
> >
> > Op 10-05-18 om 10:31 schreef Vidya Srinivas:
> > > From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > >
> > > 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
> > >
> > > v3: Moving all the src coords handling code for NV12 to
> > > skl_check_nv12_surface
> > >
> > > v4: Added RB from Mika
> > >
> > > v5: Rebased the series. Removed checks of mult of 4 in
> > > skl_update_scaler, Added NV12 condition in intel_check_sprite_plane
> > > where src x/w is being checked for mult of 2 for yuv planes.
> > >
> > > Reviewed-by: Mika Kahola <mika.kahola@intel.com>
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 42
> > > ++++++++++++++++++++++++++++++++++--
> > >  drivers/gpu/drm/i915/intel_sprite.c  |  1 +
> > >  2 files changed, 41 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index dfca71e..cca46f9 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -3102,6 +3102,42 @@ static int skl_check_main_surface(const
> > > struct
> > intel_crtc_state *crtc_state,
> > >  	return 0;
> > >  }
> > >
> > > +static int
> > > +skl_check_nv12_surface(const struct intel_crtc_state *crtc_state,
> > > +		       struct intel_plane_state *plane_state) {
> > > +	int crtc_x2 = plane_state->base.crtc_x + plane_state->base.crtc_w;
> > > +	int crtc_y2 = plane_state->base.crtc_y + plane_state->base.crtc_h;
> > > +
> > > +	if (((plane_state->base.src_x >> 16) % 4) != 0 ||
> > > +	    ((plane_state->base.src_y >> 16) % 4) != 0 ||
> > > +	    ((plane_state->base.src_w >> 16) % 4) != 0 ||
> > > +	    ((plane_state->base.src_h >> 16) % 4) != 0) {
> > > +		DRM_DEBUG_KMS("src coords must be multiple of 4 for
> > NV12\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	/* Clipping would cause a 1-3 pixel gap at the edge of the screen? */
> > > +	if ((crtc_x2 > crtc_state->pipe_src_w && crtc_state->pipe_src_w %
> > 4) ||
> > > +	    (crtc_y2 > crtc_state->pipe_src_h && crtc_state->pipe_src_h %
> > > +4))
> > {
> > > +		DRM_DEBUG_KMS("It's not possible to clip %u,%u to
> > %u,%u\n",
> > > +			      crtc_x2, crtc_y2,
> > > +			      crtc_state->pipe_src_w, crtc_state->pipe_src_h);
> > > +		return -EINVAL;
> > > +	}
> > Oops, wrong checks here..
> >
> > skl_check_nv12_surface is only needed for Display WA #1106, so check
> > might need to be something like:
> >
> > static int
> > skl_check_nv12_surface(const struct intel_crtc_state *crtc_state,
> > 		       struct intel_plane_state *plane_state) {
> > 	/* Display WA #1106 */
> > 	if (plane_state->base.rotation != (DRM_MODE_REFLECT_X |
> > DRM_MODE_ROTATE_90) &&
> > 	    plane_state->base.rotation != DRM_MODE_ROTATE_270)
> > 		return 0;
> >
> > 	/* src coordinates are rotated here. We check height but report it as
> > width. */
> > 	if (((drm_rect_height(&plane_state->base.src) >> 16) % 4) != 0) {
> > 		DRM_DEBUG_KMS("src width must be multiple of 4 for
> rotated NV12\n");
> > 		return -EINVAL;
> > 	}
> >
> > 	return 0;
> > }
> >
> > Would this hit FIFO underruns?
> 
> Thank you. I have made the change and floated the series. Please have a
> check.
> When I tested It on my end on GLK, I did not observe any fifo underruns. Will
> wait for IGT test results from BAT.
> 

IGT BAT shows PASS on rev 6 of https://patchwork.freedesktop.org/series/41674/
This has the change for skl_check_nv12_surface. Can you please have a check?
Thank you.

Regards
Vidya
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx