From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 1/2] drm/i915: Fix fbc + rc6 combination on SNB Date: Tue, 19 Nov 2013 17:40:18 +0200 Message-ID: <20131119154018.GR7819@intel.com> References: <1383350573-7427-1-git-send-email-benjamin.widawsky@intel.com> <20131102161005.GI13047@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 1F64EFCB68 for ; Tue, 19 Nov 2013 07:40:22 -0800 (PST) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Rodrigo Vivi Cc: Intel GFX , Ben Widawsky , Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org On Mon, Nov 18, 2013 at 06:08:15PM -0800, Rodrigo Vivi wrote: > I'm just on going with another -collector update and since this patch > fixes a bug I think it would be a good one to include. > = > But since it was bikeshedded it is better to ask Ville and Chris if > their comments was a NAck or I can consider to get for -collector. My FBC series makes this obsolete. I think I have a few more updates to that series that I didn't post yet. I'll try to get those out today. And I also have a crc based igt for this in the works. > = > Thanks > = > On Sat, Nov 2, 2013 at 9:10 AM, Ville Syrj=E4l=E4 > wrote: > > On Fri, Nov 01, 2013 at 05:02:52PM -0700, Ben Widawsky wrote: > >> On Sandybridge we must set the "PPGTT Render Target Base Address Valid > >> for FBC" bit as noted in the programming guide. We did this at clock > >> gating init. Thisbit is not saved and restored with RC6 power context, > >> so the resetting it at ring flush should fix that. > >> > >> The effect of not doing this should be corruption, and not a hang - as > >> has so often been the case. > >> > >> Note that we should actually clear this bit as well when not blitting = to > >> the scanout (using the blitter for other things), or else all operatio= ns > >> > >> Cc: St=E9phane Marchesin > >> Signed-off-by: Ben Widawsky > >> --- > >> drivers/gpu/drm/i915/intel_pm.c | 2 -- > >> drivers/gpu/drm/i915/intel_ringbuffer.c | 25 +++++++++++++++++++++++++ > >> 2 files changed, 25 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/in= tel_pm.c > >> index ca9a778..67f460b 100644 > >> --- a/drivers/gpu/drm/i915/intel_pm.c > >> +++ b/drivers/gpu/drm/i915/intel_pm.c > >> @@ -193,8 +193,6 @@ static void sandybridge_blit_fbc_update(struct drm= _device *dev) > >> /* Make sure blitter notifies FBC of writes */ > >> gen6_gt_force_wake_get(dev_priv); > >> blt_ecoskpd =3D I915_READ(GEN6_BLITTER_ECOSKPD); > >> - blt_ecoskpd |=3D GEN6_BLITTER_FBC_NOTIFY << > >> - GEN6_BLITTER_LOCK_SHIFT; > >> I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd); > >> blt_ecoskpd |=3D GEN6_BLITTER_FBC_NOTIFY; > >> I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd); > > > > Why leave the other FBC_NOTIFY frobbing in place? Since you don't set > > the mask bit anymore the rest isn't guaranteed to do anything. > > > >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm= /i915/intel_ringbuffer.c > >> index 2dec134..ddd7681 100644 > >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > >> @@ -278,6 +278,28 @@ gen7_render_ring_cs_stall_wa(struct intel_ring_bu= ffer *ring) > >> return 0; > >> } > >> > >> +static int gen6_ring_fbc_flush(struct intel_ring_buffer *ring) > >> +{ > >> + int ret; > >> + > >> + if (!ring->fbc_dirty) > >> + return 0; > >> + > >> + ret =3D intel_ring_begin(ring, 4); > >> + if (ret) > >> + return ret; > >> + > >> + intel_ring_emit(ring, MI_NOOP); > >> + intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1)); > >> + intel_ring_emit(ring, GEN6_BLITTER_ECOSKPD); > >> + intel_ring_emit(ring, > >> + _MASKED_BIT_ENABLE(GEN6_BLITTER_FBC_NOTIFY)); > >> + intel_ring_advance(ring); > >> + > >> + ring->fbc_dirty =3D false; > >> + return 0; > >> +} > >> + > >> static int gen7_ring_fbc_flush(struct intel_ring_buffer *ring, u32 va= lue) > >> { > >> int ret; > >> @@ -1712,6 +1734,9 @@ static int gen6_ring_flush(struct intel_ring_buf= fer *ring, > >> intel_ring_emit(ring, MI_NOOP); > >> intel_ring_advance(ring); > >> > >> + if (IS_GEN6(dev) && flush) > >> + return gen6_ring_fbc_flush(ring); > >> + > > > > What Chris said about doing this before the batch is dispatched. > > > > Afer a bit of thought I think the following logic would work nicely: > > > > execbuffer() { > > ring->new_fbc_obj =3D NULL; > > for_each_obj() { > > if (is_crtc_fb(obj) && obj.write_domains) > > ring->new_fbc_obj =3D obj; > > if (gen >=3D 7) { > > if (ring->new_fbc_obj) > > ring->fbc_dirty =3D true; > > } else { > > if (ring->new_fb_obj !=3D ring->fbc_obj) { > > ring->fbc_obj =3D ring->new_fbc_obj; > > ring->fbc_dirty =3D true; > > } > > } > > > > invalidate() { > > if (gen < 7 && ring->fbc_dirty) { > > if (ring->fbc_obj) > > enable_tracking; > > else > > disable_tracking; > > } > > } > > > > dispatch() > > > > flush() { > > if (gen >=3D 7 && ring->fbc_dirty) > > fbc_nuke(); > > ring->fbc_dirty =3D false; > > } > > } > > > > I think that same logic would work for both blitter and render. The > > difference between the two is that for render we also need to update > > the address, for blitter we just need to set the notify bit. > > > > Also since we could update the render tracking for every batch, the > > problem of having the render fbc tracking address in the context > > would also be solved by simply setting fbc_dirty=3Dtrue on context > > switch. > > > > I don't recall excatly how we're supposed to do blitter tracking on > > on gen7+. I seem to recall that it also had a nuke mechanism, but > > I don't see it being used in out code ATM. > > > > -- > > Ville Syrj=E4l=E4 > > Intel OTC > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > = > = > = > -- = > Rodrigo Vivi > Blog: http://blog.vivi.eng.br -- = Ville Syrj=E4l=E4 Intel OTC