> -----Original Message----- > From: Maarten Lankhorst [mailto:maarten.lankhorst@xxxxxxxxxxxxxxx] > Sent: Thursday, April 5, 2018 3:32 PM > To: Srinivas, Vidya <vidya.srinivas@xxxxxxxxx>; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx > Cc: Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx> > Subject: Re: [PATCH v19 18/18] drm/i915: Set src size restrictions > for NV12 > > Op 05-04-18 om 07:18 schreef Srinivas, Vidya: > > > >> -----Original Message----- > >> From: Maarten Lankhorst [mailto:maarten.lankhorst@xxxxxxxxxxxxxxx] > >> Sent: Wednesday, April 4, 2018 2:37 PM > >> To: Srinivas, Vidya <vidya.srinivas@xxxxxxxxx>; intel- > >> gfx@xxxxxxxxxxxxxxxxxxxxx > >> Cc: Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx> > >> Subject: Re: [PATCH v19 18/18] drm/i915: Set src size > >> restrictions for NV12 > >> > >> Op 04-04-18 om 10:51 schreef Srinivas, Vidya: > >>>> -----Original Message----- > >>>> From: Maarten Lankhorst [mailto:maarten.lankhorst@xxxxxxxxxxxxxxx] > >>>> Sent: Wednesday, April 4, 2018 2:18 PM > >>>> To: Srinivas, Vidya <vidya.srinivas@xxxxxxxxx>; intel- > >>>> gfx@xxxxxxxxxxxxxxxxxxxxx > >>>> Cc: Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx> > >>>> Subject: Re: [PATCH v19 18/18] drm/i915: Set src size > >>>> restrictions for NV12 > >>>> > >>>> Op 04-04-18 om 10:04 schreef Srinivas, Vidya: > >>>>>> -----Original Message----- > >>>>>> From: Maarten Lankhorst > >>>>>> [mailto:maarten.lankhorst@xxxxxxxxxxxxxxx] > >>>>>> Sent: Wednesday, April 4, 2018 1:28 PM > >>>>>> To: Srinivas, Vidya <vidya.srinivas@xxxxxxxxx>; intel- > >>>>>> gfx@xxxxxxxxxxxxxxxxxxxxx > >>>>>> Cc: Lankhorst, Maarten <maarten.lankhorst@xxxxxxxxx> > >>>>>> Subject: Re: [PATCH v19 18/18] drm/i915: Set src size > >>>>>> restrictions for NV12 > >>>>>> > >>>>>> Op 02-04-18 om 11:51 schreef Vidya Srinivas: > >>>>>>> As per display WA 1106, to avoid corruption issues > >>>>>>> NV12 plane height needs to be multiplier of 4 We expect the src > >>>>>>> dimensions to be multiplier of 4 We fail the case where src > >>>>>>> width or height is not multiple of 4 for NV12. > >>>>>>> We also set the scaler destination height and width to be > >>>>>>> multiple of 4. Without this, pipe fifo underruns were seen on APL > and KBL. > >>>>>>> We also skip src trunction/adjustments for NV12 case and handle > >>>>>>> the sizes directly in skl_update_plane > >>>>>>> > >>>>>>> Credits-to: Maarten Lankhorst > >>>>>>> <maarten.lankhorst@xxxxxxxxxxxxxxx> > >>>>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@xxxxxxxxx> > >>>>>>> --- > >>>>>>> drivers/gpu/drm/i915/intel_drv.h | 2 ++ > >>>>>>> drivers/gpu/drm/i915/intel_sprite.c | 19 ++++++++++++++++--- > >>>>>>> 2 files changed, 18 insertions(+), 3 deletions(-) > >>>>>>> > >>>>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h > >>>>>>> b/drivers/gpu/drm/i915/intel_drv.h > >>>>>>> index 9c58da0..a1f718d 100644 > >>>>>>> --- a/drivers/gpu/drm/i915/intel_drv.h > >>>>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h > >>>>>>> @@ -159,6 +159,8 @@ > >>>>>>> #define INTEL_I2C_BUS_DVO 1 > >>>>>>> #define INTEL_I2C_BUS_SDVO 2 > >>>>>>> > >>>>>>> +#define MULT4(x) ((x + 3) & ~0x03) > >>>>>>> + > >>>>>>> /* these are outputs from the chip - integrated only > >>>>>>> external chips are via DVO or SDVO output */ enum > >>>>>>> intel_output_type { diff --git > >>>>>>> a/drivers/gpu/drm/i915/intel_sprite.c > >>>>>>> b/drivers/gpu/drm/i915/intel_sprite.c > >>>>>>> index d5dad44..b2292dd 100644 > >>>>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c > >>>>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c > >>>>>>> @@ -255,6 +255,12 @@ skl_update_plane(struct intel_plane > *plane, > >>>>>>> uint32_t src_h = drm_rect_height(&plane_state->base.src) > >> 16; > >>>>>>> unsigned long irqflags; > >>>>>>> > >>>>>>> + if (fb->format->format == DRM_FORMAT_NV12 && > >>>>>>> + ((src_h % 4) != 0 || (src_w % 4) != 0)) { > >>>>>>> + DRM_DEBUG_KMS("NV12: src dimensions not > >> valid\n"); > >>>>>>> + return; > >>>>>>> + } > >>>>>>> + > >>>>>> You can't do this check in skl_update_plane, it's way too late. > >>>>>> It should be rejected in the plane check with -EINVAL, or perhaps > >>>>>> in > >>>> skl_update_scaler. > >>>>> Have done it right from intel_framebuffer_init onwards, have done > >>>>> it in skl_update_scaler also In fact it will get rejected in > >>>>> framebuffer init and > >>>> all these are duplicate checks, can remove them. > >>>>>>> /* Sizes are 0 based */ > >>>>>>> src_w--; > >>>>>>> src_h--; > >>>>>>> @@ -292,9 +298,12 @@ skl_update_plane(struct intel_plane > *plane, > >>>>>>> PS_SCALER_EN | > PS_PLANE_SEL(plane_id) | > >>>>>> scaler->mode); > >>>>>>> I915_WRITE_FW(SKL_PS_PWR_GATE(pipe, > scaler_id), 0); > >>>>>>> I915_WRITE_FW(SKL_PS_WIN_POS(pipe, scaler_id), > (crtc_x > >>>>>> << 16) | crtc_y); > >>>>>>> - I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id), > >>>>>>> - ((crtc_w + 1) << 16)|(crtc_h + 1)); > >>>>>>> - > >>>>>>> + if (fb->format->format == DRM_FORMAT_NV12) > >>>>>>> + I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, > >> scaler_id), > >>>>>>> + (MULT4(crtc_w) << 16) | > >>>>>> MULT4(crtc_h)); > >>>>>> See the comment above, sizes are 0 based. This will add 1 to the > >>>>>> size, so the size is always 1 more than requested. > >>>>>> I don't think it would pass plane CRC tests.. > >>>>> When I align it to mult of 4, 1919 (which was 1920-1) becomes 1920 > >> back. > >>>>> If we don’t do this, I see fifo underruns. The destination width > >>>>> and height If not mult of 4, I am seeing underruns. > >>>> What you see as 1920 is 1921, which should be underrunning even > >>>> more and is out of FB bounds. > >>> Sorry, I gave a wrong example here. It doesn’t happen when we scale > >>> it to > >> full resolution. > >>> It happened during other testing when the scaled dest width or > >>> height was not multiple of 4. > >>> > >> We could reject it, but odd that crtc w/h matters. I didn't expect > >> that. :) > >> > >> Rejecting with -EINVAL is the best way to go, along with > >> documentation in the form of tests that we handle this case correctly. > > Hi Maarten, > > > > Completely understand and agree. With these fifo underruns, I have > > tried too many tests and encountered those fifo underruns under diff > > set of experiments. I remember seeing them when src width and height > > were not x4. When I kept src dimensions x4, I hit underruns during > > destination testing. But right now, I tried to reproduce the same - it > > isn’t happening on my APL. So, I have removed all restrictions from > > the series https://patchwork.freedesktop.org/series/38919/ > > (sent to trybot only for now). I have just retained the src height to > > be min 16 which Is mentioned in bspec. Now since we have i-g-t 16x16 > > merged and the only change for > > NV12 majorly for underruns is we skip the truncations that were > > happening in intel_check_sprite We will get CI results shortly. Due to > > too many experiments done at my end, I overloaded myself with data and > get confused - when I hit which issue. Apologies for the same. > > PS: I removed the src restrictions too in this series. Because during > > clipping clamping tests, the fb src widthxheight generated is like 257x159. > This gets rejected from KMD as its not x4. > I've done testing and had very reliable underruns with height not a multiple > of 2, as could be seen in the earlier kms_nv12 series. Clipping/clamping > tests should be fixed at least not to get into such a bad situation for NV12, > and take alignment into account. Sure, will address the same. Thank you. Regards Vidya > > ~Maarten _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
