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

Tue Apr 17 10:18:27 UTC 2018

> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
> Sent: Tuesday, April 17, 2018 3:32 PM
> To: Srinivas, Vidya <vidya.srinivas at intel.com>; Saarinen, Jani
> <jani.saarinen at intel.com>; intel-gfx at lists.freedesktop.org
> Cc: Kamath, Sunil <sunil.kamath at intel.com>
> Subject: Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments for
> NV12
> 
> Op 17-04-18 om 11:42 schreef Srinivas, Vidya:
> >
> >> -----Original Message-----
> >> From: Maarten Lankhorst [mailto:maarten.lankhorst at linux.intel.com]
> >> Sent: Tuesday, April 17, 2018 3:04 PM
> >> To: Srinivas, Vidya <vidya.srinivas at intel.com>; Saarinen, Jani
> >> <jani.saarinen at intel.com>; intel-gfx at lists.freedesktop.org
> >> Cc: Kamath, Sunil <sunil.kamath at intel.com>
> >> Subject: Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments
> >> for
> >> NV12
> >>
> >> Op 17-04-18 om 10:09 schreef Srinivas, Vidya:
> >>>> -----Original Message-----
> >>>> From: Srinivas, Vidya
> >>>> Sent: Tuesday, April 17, 2018 1:32 PM
> >>>> To: 'Maarten Lankhorst' <maarten.lankhorst at linux.intel.com>;
> >>>> Saarinen, Jani <jani.saarinen at intel.com>;
> >>>> intel-gfx at lists.freedesktop.org
> >>>> Cc: Kamath, Sunil <sunil.kamath 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: Tuesday, April 17, 2018 1:21 PM
> >>>>> To: Srinivas, Vidya <vidya.srinivas at intel.com>; Saarinen, Jani
> >>>>> <jani.saarinen at intel.com>; intel-gfx at lists.freedesktop.org
> >>>>> Cc: Kamath, Sunil <sunil.kamath at intel.com>
> >>>>> Subject: Re: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments
> >>>>> for
> >>>>> NV12
> >>>>>
> >>>>> Op 17-04-18 om 09:38 schreef Srinivas, Vidya:
> >>>>>>> -----Original Message-----
> >>>>>>> From: Maarten Lankhorst
> >>>>>>> [mailto:maarten.lankhorst at linux.intel.com]
> >>>>>>> Sent: Tuesday, April 17, 2018 12:55 PM
> >>>>>>> To: Saarinen, Jani <jani.saarinen at intel.com>; Srinivas, Vidya
> >>>>>>> <vidya.srinivas at intel.com>;
> >>>>>>> intel-gfx-trybot at lists.freedesktop.org;
> >>>>>>> intel- gfx at lists.freedesktop.org
> >>>>>>> Cc: Kamath, Sunil <sunil.kamath at intel.com>
> >>>>>>> Subject: Re: [PATCH v2 6/6] drm/i915: Do not do fb src
> >>>>>>> adjustments for
> >>>>>>> NV12
> >>>>>>>
> >>>>>>> Op 17-04-18 om 08:03 schreef 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.
> >>>>>>> Correct. This is why we fix up to a multiple of 4 after
> >>>>>>> adjustments, and reject user passed coordinates before
> adjustment..
> >>>>>>> plane_state->src_x/y/w/h is the property set by the user, while
> >>>>>>> the plane_state->src rect is set and fixed up in the kernel.
> >>>>>>>
> >>>>>>> Rejecting clipping with positive coordinates if pipe_src w/h is
> >>>>>>> not a multiple of 4 will fix a border case that shouldn't affect
> >>>>>>> most modes, where the plane wouldn't extend to the crtc borders,
> >>>>>>> which is
> >>>>> what userspace wants there.
> >>>>>> Thank you. Oh okay. Sorry. I thought we just reject and not do
> >>>>>> the fixing
> >>>>> (mult of 4 later).
> >>>>>> Sure, I will change the patch to also reject the coordinates
> >>>>>> before adjustment (if its not Mult of 4).
> >>>>>>
> >>>>>>
> >>>>> This can be done in skl_check_plane_surface,
> >>>>> plane_state->src_{x,y,w,h} contains the user requested values.
> >>>> Apologies for asking again. The adjustments are done in check_plane
> >>>> (both primary and sprite) And towards the end of check_plane we
> >>>> call the skl_check_plane_surface.
> >>>> I got your point that we will reject the user requested values if
> >>>> they are not multiple of 4.
> >>>> But before we reach skl_check_plane_surface, they would’ve been
> >>>> adjusted/clipped etc They wont remain user requested as is when
> >>>> they enter skl_check_plane_surface.
> >>>> Please correct me if I am wrong or I am missing in understanding
> >>>> something here :(
> >>>>
> >>>> I can add the add conversion to multiple of 4 in
> >>>> skl_check_plane_surface but rejection should be done before the
> >>>> clipping/adjustment Or else, I move the entire clipping/adjustment
> >>>> code
> >> to skl_check_plane_surface?
> >>> Sorry, I got it. You said, we don’t do adjustments in case of NV12
> >>> in sprite and move The handling to skl_check_plane_surface. I will
> >>> do the
> >> change and submit. Sorry.
> >>> Thank you.
> >>>
> >> Something like below? Untested..
> >> We should probably also restrict NV12 fb creation to not allow CCS
> formats.
> >>
> >> ~Maarten
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c
> >> b/drivers/gpu/drm/i915/intel_display.c
> >> index 4b3735720fee..c5da30c7ad04 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -3090,6 +3090,38 @@ 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;
> >> +
> >> +	/* Are the unadjusted coordinates passed in valid? */
> >> +	if ((plane_state->base.src_x >> 16) % 4 ||
> >> +	    (plane_state->base.src_y >> 16) % 4 ||
> >> +	    (plane_state->base.src_w >> 16) % 4 ||
> >> +	    (plane_state->base.src_h >> 16) % 4) {
> >> +		DRM_DEBUG_KMS("Source coordinates must be a 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;
> >> +	}
> >> +
> >> +	/* Make adjusted src coordinates a multiple of 4. */
> >> +	plane_state->base.src.x1 = DIV_ROUND_CLOSEST(plane_state-
> >>> base.src.x1, 1 << 18) << 18;
> >> +	plane_state->base.src.y1 = DIV_ROUND_CLOSEST(plane_state-
> >>> base.src.y1, 1 << 18) << 18;
> >> +	plane_state->base.src.x2 = DIV_ROUND_CLOSEST(plane_state-
> >>> base.src.x2, 1 << 18) << 18;
> >> +	plane_state->base.src.y2 = DIV_ROUND_CLOSEST(plane_state-
> >>> base.src.y2, 1 << 18) << 18;
> >> +	return 0;
> >> +}
> >> +
> > Thank you so much :) Only one problem. The primary plane clipping
> > happens inside the drm layer in drm_atomic_helper_check_plane_state
> > So, clipping-clamping test passes coordinates 0, 257, 0, 159 to
> check_plane.
> > And there before adjusting we reject. So for primary alone, can I
> > retain the make multiple of 4 code in primary itself?
> 
> The clipping affects plane_state->src.{x1, y1, x2, y2}, not plane_state-
> >src_{x,y,w,h}.
> They're different. The last set of coordinates will not be rotated or anything,
> the first set of coordinates are what we end up programming into the
> hardware. The patch I put above should be correctly rounding to a multiple
> of 4. :)

Oh sorry. I will put this patch. Thanks so much.

> 
> ~Maarten