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. ~Maarten _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
