From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rodrigo Vivi Subject: Re: [PATCH] drm/i915: WA: FBC Render Nuke. Date: Mon, 3 Jun 2013 13:50:46 -0300 Message-ID: References: <1369787112-5850-1-git-send-email-rodrigo.vivi@gmail.com> <20130531155936.GY5004@intel.com> <20130603113459.GA5004@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-vb0-f46.google.com (mail-vb0-f46.google.com [209.85.212.46]) by gabe.freedesktop.org (Postfix) with ESMTP id DD3F0E60C6 for ; Mon, 3 Jun 2013 09:50:46 -0700 (PDT) Received: by mail-vb0-f46.google.com with SMTP id 10so953663vbe.5 for ; Mon, 03 Jun 2013 09:50:46 -0700 (PDT) In-Reply-To: <20130603113459.GA5004@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: =?ISO-8859-1?Q?Ville_Syrj=E4l=E4?= Cc: intel-gfx List-Id: intel-gfx@lists.freedesktop.org On Mon, Jun 3, 2013 at 8:34 AM, Ville Syrj=E4l=E4 wrote: > On Fri, May 31, 2013 at 05:15:41PM -0300, Rodrigo Vivi wrote: >> Hi Ville, >> >> Thanks for the comments. >> >> >> On Fri, May 31, 2013 at 12:59 PM, Ville Syrj=E4l=E4 >> wrote: >> > On Tue, May 28, 2013 at 09:25:12PM -0300, Rodrigo Vivi wrote: >> >> WaFbcNukeOn3DBlt for IVB, HSW and VLV. >> > >> > VLV doesn't have FBC, so this is a bit incorrect. >> >> I'm going to remove the vlv mention that incorrectly came from spec... >> >> > >> >> >> >> According BSPec: "Workaround: Do not enable Render Command Streamer t= racking for FBC. >> >> Instead insert a LRI to address 0x50380 with data 0x00000004 after th= e PIPE_CONTROL that >> >> follows each render submission." >> >> >> >> v2: Chris noticed that flush_domains check was missing here and also = suggested to do >> >> LRI only when fbc is enabled. To avoid do a I915_READ on every fl= ush lets use the >> >> module parameter check. >> >> >> >> v3: Adding Wa name as Damien suggested. >> >> >> >> Cc: Chris Wilson >> >> Signed-off-by: Rodrigo Vivi >> >> --- >> >> drivers/gpu/drm/i915/i915_reg.h | 2 ++ >> >> drivers/gpu/drm/i915/intel_pm.c | 2 +- >> >> drivers/gpu/drm/i915/intel_ringbuffer.c | 32 +++++++++++++++++++++++= +++++++++ >> >> 3 files changed, 35 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i= 915_reg.h >> >> index cc4c223..81ac584 100644 >> >> --- a/drivers/gpu/drm/i915/i915_reg.h >> >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> >> @@ -977,6 +977,8 @@ >> >> /* Framebuffer compression for Ivybridge */ >> >> #define IVB_FBC_RT_BASE 0x7020 >> >> >> >> +#define MSG_FBC_REND_STATE 0x50380 >> >> +#define FBC_REND_NUKE (1<<2) >> >> >> >> #define _HSW_PIPE_SLICE_CHICKEN_1_A 0x420B0 >> >> #define _HSW_PIPE_SLICE_CHICKEN_1_B 0x420B4 >> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/i= ntel_pm.c >> >> index 1879188..e830a9b 100644 >> >> --- a/drivers/gpu/drm/i915/intel_pm.c >> >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> >> @@ -274,7 +274,7 @@ static void gen7_enable_fbc(struct drm_crtc *crtc= , unsigned long interval) >> >> struct drm_i915_gem_object *obj =3D intel_fb->obj; >> >> struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); >> >> >> >> - I915_WRITE(IVB_FBC_RT_BASE, obj->gtt_offset | ILK_FBC_RT_VALID); >> >> + I915_WRITE(IVB_FBC_RT_BASE, obj->gtt_offset); >> >> >> >> if (!intel_edp_is_psr_enabled(dev)) >> >> I915_WRITE(ILK_DPFC_CONTROL, DPFC_CTL_EN | DPFC_CTL_LIM= IT_1X | >> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/dr= m/i915/intel_ringbuffer.c >> >> index 3d2c236..69491db 100644 >> >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c >> >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c >> >> @@ -280,6 +280,30 @@ gen7_render_ring_cs_stall_wa(struct intel_ring_b= uffer *ring) >> >> return 0; >> >> } >> >> >> >> +static int gen7_ring_fbc_flush(struct intel_ring_buffer *ring) >> >> +{ >> >> + struct drm_device *dev =3D ring->dev; >> >> + int ret; >> >> + >> >> + if (i915_enable_fbc =3D=3D 0) >> >> + return 0; >> >> + >> >> + if (i915_enable_fbc < 0 && !IS_HASWELL(dev)) >> >> + return 0; >> >> + >> >> + ret =3D intel_ring_begin(ring, 4); >> >> + if (ret) >> >> + return ret; >> >> + intel_ring_emit(ring, MI_NOOP); >> >> + /* WaFbcNukeOn3DBlt:ivb/hsw/vlv */ >> > >> > Another mention of vlv. I can see BSpec makes the same mistake in >> > the register description though. >> >> ... as you noticed. >> >> > >> >> + intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1)); >> >> + intel_ring_emit(ring, MSG_FBC_REND_STATE); >> >> + intel_ring_emit(ring, FBC_REND_NUKE); >> >> + intel_ring_advance(ring); >> >> + >> >> + return 0; >> >> +} >> >> + >> >> static int >> >> gen7_render_ring_flush(struct intel_ring_buffer *ring, >> >> u32 invalidate_domains, u32 flush_domains) >> >> @@ -336,6 +360,9 @@ gen7_render_ring_flush(struct intel_ring_buffer *= ring, >> >> intel_ring_emit(ring, 0); >> >> intel_ring_advance(ring); >> >> >> >> + if (flush_domains) >> >> + return gen7_ring_fbc_flush(ring); >> >> + >> >> return 0; >> >> } >> >> >> >> @@ -1623,6 +1650,7 @@ gen6_ring_dispatch_execbuffer(struct intel_ring= _buffer *ring, >> >> static int blt_ring_flush(struct intel_ring_buffer *ring, >> >> u32 invalidate, u32 flush) >> >> { >> >> + struct drm_device *dev =3D ring->dev; >> >> uint32_t cmd; >> >> int ret; >> >> >> >> @@ -1645,6 +1673,10 @@ static int blt_ring_flush(struct intel_ring_bu= ffer *ring, >> >> intel_ring_emit(ring, 0); >> >> intel_ring_emit(ring, MI_NOOP); >> >> intel_ring_advance(ring); >> >> + >> >> + if (IS_GEN7(dev)) >> >> + return gen7_ring_fbc_flush(ring); >> > >> > Should check flush_domains here as well? >> >> here is flush domain by definition, isn' t it? > > How so? this function is the ring->flush. how is it possible to have it out of flush domain? or are the names just confusing me? > >> >> > >> > So we're now using the same nuke mechanism from the blt ring too. >> > Should we then drop the regular blitter tracking things from fbc_enabl= e? >> >> This is a good question. Since this is a critical patch and it is >> working in the way it is I prefer to let it in the way it is and >> promisse that I will try to drop old blitter tracking for ivb and hsw >> later. If it works I'll send the drop in another patch. >> >> > >> > Oh and what about vcs and vecs, should we nuke from those rings as wel= l? >> > I guess it would be strange to write to the primary plane's buffer via >> > vcs, but I'm assuming vebox could write the same formats that we can >> > scan out... >> >> To be truly honest with you I have no idea about these case. specs >> just says to put after every pipe_control following flush >> renderings... and blt. > > IIRC the spec doesn't say anything about blt. > > Ah, the PM giude tells you to do LRIs w/ blt too. But it actually says > that you should do "cache clean" LRIs insted of "nuke" LRIs. It is just the name, but same address and bit. from BSPec: " Driver must program a MI_FLUSH_DW followed by a LRI into the BCS ring to generate a cache clean message to FBC (LRI to offset 0x50380 with data 0x00000002)." With this blit in the way it is I' m getting best rendering and power saving performance. > >> >> > >> >> + >> >> return 0; >> >> } >> >> >> >> -- >> >> 1.8.1.4 >> >> >> >> _______________________________________________ >> >> Intel-gfx mailing list >> >> Intel-gfx@lists.freedesktop.org >> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> > >> > -- >> > Ville Syrj=E4l=E4 >> > Intel OTC >> >> >> >> -- >> Rodrigo Vivi >> Blog: http://blog.vivi.eng.br > > -- > Ville Syrj=E4l=E4 > Intel OTC -- Rodrigo Vivi Blog: http://blog.vivi.eng.br