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

[Saarinen, Jani]

Hi, 

> -----Original Message-----
> From: Srinivas, Vidya
> Sent: keskiviikko 18. huhtikuuta 2018 6.56
> To: Saarinen, Jani <jani.saarinen at intel.com>; Maarten Lankhorst
> <maarten.lankhorst at linux.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: Saarinen, Jani
> > Sent: Tuesday, April 17, 2018 4:23 PM
> > To: Srinivas, Vidya <vidya.srinivas at intel.com>; Maarten Lankhorst
> > <maarten.lankhorst at linux.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
> >
> > HI,
> > > -----Original Message-----
> > > From: Srinivas, Vidya
> > > Sent: tiistai 17. huhtikuuta 2018 13.49
> > > 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 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.
> > >
> > > Thank you so much. Have submitted the changes to trybot
> > > https://patchwork.freedesktop.org/series/41532/
> > > PS: fb creation to not allow CCS - not done in this series
> > Why not directly to intel-gfx?
> Sure, I thought I had to send to trybot first. Will send it today.
I see trybot for patches as test for RFC to see if solution is even near satisfactory in the beginning but for patches that has sent several times no need to send to trybot. 



> 
> Thanks
> Vidya
> >
> >
> > >
> > > Regards
> > > Vidya
> > >
> > > > >>
> > > > >> ~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. :)
> > > >
> > > > ~Maarten