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

Wed Apr 18 05:55:19 UTC 2018

> -----Original Message-----
> From: Saarinen, Jani
> Sent: Wednesday, April 18, 2018 11:07 AM
> 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: 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.

Oh okay. Sure, thank you so much.

Regards
Vidya
> 
> 
> 
> >
> > 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