> -----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? > static int skl_check_nv12_aux_surface(struct intel_plane_state > *plane_state) { > const struct drm_framebuffer *fb = plane_state->base.fb; @@ - > 3162,6 +3194,12 @@ int skl_check_plane_surface(const struct > intel_crtc_state *crtc_state, > if (!plane_state->base.visible) > return 0; > > + if (fb->format->format == DRM_FORMAT_NV12) { > + ret = skl_check_nv12_surface(crtc_state, plane_state); > + if (ret) > + return ret; > + } > + > /* Rotate src coordinates to match rotated GTT view */ > if (drm_rotation_90_or_270(rotation)) > drm_rect_rotate(&plane_state->base.src, _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
