Op 17-04-18 om 11:42 schreef Srinivas, Vidya: > >> -----Original Message----- >> From: Maarten Lankhorst [mailto:maarten.lankhorst@xxxxxxxxxxxxxxx] >> Sent: Tuesday, April 17, 2018 3:04 PM >> To: Srinivas, Vidya <vidya.srinivas@xxxxxxxxx>; Saarinen, Jani >> <jani.saarinen@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> Cc: Kamath, Sunil <sunil.kamath@xxxxxxxxx> >> 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@xxxxxxxxxxxxxxx>; >>>> Saarinen, Jani <jani.saarinen@xxxxxxxxx>; >>>> intel-gfx@xxxxxxxxxxxxxxxxxxxxx >>>> Cc: Kamath, Sunil <sunil.kamath@xxxxxxxxx> >>>> Subject: RE: [PATCH v2 6/6] drm/i915: Do not do fb src adjustments >>>> for >>>> NV12 >>>> >>>> >>>> >>>>> -----Original Message----- >>>>> From: Maarten Lankhorst [mailto:maarten.lankhorst@xxxxxxxxxxxxxxx] >>>>> Sent: Tuesday, April 17, 2018 1:21 PM >>>>> To: Srinivas, Vidya <vidya.srinivas@xxxxxxxxx>; Saarinen, Jani >>>>> <jani.saarinen@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx >>>>> Cc: Kamath, Sunil <sunil.kamath@xxxxxxxxx> >>>>> 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@xxxxxxxxxxxxxxx] >>>>>>> Sent: Tuesday, April 17, 2018 12:55 PM >>>>>>> To: Saarinen, Jani <jani.saarinen@xxxxxxxxx>; Srinivas, Vidya >>>>>>> <vidya.srinivas@xxxxxxxxx>; >>>>>>> intel-gfx-trybot@xxxxxxxxxxxxxxxxxxxxx; >>>>>>> intel- gfx@xxxxxxxxxxxxxxxxxxxxx >>>>>>> Cc: Kamath, Sunil <sunil.kamath@xxxxxxxxx> >>>>>>> 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@xxxxxxxxxxxxxxx>; >>>>>>>>> intel-gfx- trybot@xxxxxxxxxxxxxxxxxxxxx >>>>>>>>> Cc: Kamath, Sunil <sunil.kamath@xxxxxxxxx>; Saarinen, Jani >>>>>>>>> <jani.saarinen@xxxxxxxxx> >>>>>>>>> Subject: RE: [PATCH v2 6/6] drm/i915: Do not do fb src >>>>>>>>> adjustments for NV12 >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>>> -----Original Message----- >>>>>>>>>> From: Maarten Lankhorst >>>>>>>>>> [mailto:maarten.lankhorst@xxxxxxxxxxxxxxx] >>>>>>>>>> Sent: Monday, April 16, 2018 8:00 PM >>>>>>>>>> To: Srinivas, Vidya <vidya.srinivas@xxxxxxxxx>; intel-gfx- >>>>>>>>>> trybot@xxxxxxxxxxxxxxxxxxxxx >>>>>>>>>> Cc: Kamath, Sunil <sunil.kamath@xxxxxxxxx>; Saarinen, Jani >>>>>>>>>> <jani.saarinen@xxxxxxxxx> >>>>>>>>>> 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@xxxxxxxxxxxxxxx> >>>>>>>>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@xxxxxxxxx> >>>>>>>>>>> --- >>>>>>>>>>> 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. :) ~Maarten _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
