> -----Original Message----- > From: Saarinen, Jani > Sent: Tuesday, April 17, 2018 4:23 PM > To: Srinivas, Vidya <vidya.srinivas@xxxxxxxxx>; Maarten Lankhorst > <maarten.lankhorst@xxxxxxxxxxxxxxx>; 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 > > HI, > > -----Original Message----- > > From: Srinivas, Vidya > > Sent: tiistai 17. huhtikuuta 2018 13.49 > > 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 3:32 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 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. > > > > 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. 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
