From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 3/6] drm/i915: Implement LRI based FBC tracking Date: Wed, 6 Nov 2013 18:24:19 +0200 Message-ID: <20131106162419.GG5986@intel.com> References: <1383752345-10178-1-git-send-email-ville.syrjala@linux.intel.com> <1383752345-10178-3-git-send-email-ville.syrjala@linux.intel.com> <20131106160629.GA26123@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id 7A31FF0FAC for ; Wed, 6 Nov 2013 08:24:35 -0800 (PST) Content-Disposition: inline In-Reply-To: <20131106160629.GA26123@nuc-i3427.alporthouse.com> 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: Chris Wilson , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Wed, Nov 06, 2013 at 04:06:29PM +0000, Chris Wilson wrote: > On Wed, Nov 06, 2013 at 05:39:02PM +0200, ville.syrjala@linux.intel.com w= rote: > > From: Ville Syrj=E4l=E4 > > = > > As per the SNB and HSW PM guides, we should enable FBC render/blitter > > tracking only during batches targetting the front buffer. > > = > > On SNB we must also update the FBC render tracking address whenever it > > changes. And since the register in question is stored in the context, > > we need to make sure we reload it with correct data after context > > switches. > > = > > On IVB/HSW we use the render nuke mechanism, so no render tracking > > address updates are needed. Hoever on the blitter side we need to > > enable the blitter tracking like on SNB, and in addition we need > > to issue the cache clean messages, which we already did. > > = > > Signed-off-by: Ville Syrj=E4l=E4 > > --- > > drivers/gpu/drm/i915/i915_gem_context.c | 7 ++++ > > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 31 ++++++++++++++++ > > drivers/gpu/drm/i915/intel_display.c | 28 +++++++++++++-- > > drivers/gpu/drm/i915/intel_drv.h | 3 ++ > > drivers/gpu/drm/i915/intel_pm.c | 2 -- > > drivers/gpu/drm/i915/intel_ringbuffer.c | 58 ++++++++++++++++++++++= +++++++- > > drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++ > > 7 files changed, 126 insertions(+), 5 deletions(-) > > = > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/= i915/i915_gem_context.c > > index 72a3df3..d438ea1 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > @@ -404,6 +404,13 @@ mi_set_context(struct intel_ring_buffer *ring, > > = > > intel_ring_advance(ring); > > = > > + /* > > + * FBC RT address is stored in the context, so we may have just > > + * restored it to an old value. Make sure we emit a new LRI > > + * to update the address. > > + */ > > + ring->fbc_address_dirty =3D true; > > + > > return ret; > > } > > = > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/d= rm/i915/i915_gem_execbuffer.c > > index 885d595..a8f8634 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > @@ -886,6 +886,35 @@ validate_exec_list(struct drm_i915_gem_exec_object= 2 *exec, > > } > > = > > static void > > +i915_gem_execbuffer_mark_fbc_dirty(struct intel_ring_buffer *ring, > > + struct list_head *vmas) > > +{ > > + struct i915_vma *vma; > > + struct drm_i915_gem_object *fbc_obj =3D NULL; > > + u32 fbc_address =3D -1; > > + > > + list_for_each_entry(vma, vmas, exec_list) { > > + struct drm_i915_gem_object *obj =3D vma->obj; > > + > > + if (obj->base.pending_write_domain =3D=3D 0) > > + continue; > > + > > + if (obj->pin_count) /* check for potential scanout */ > > + intel_mark_fbc_dirty(obj, ring, &fbc_obj); > = > if (obj->pin_display && intel_fbc_active(obj)) > fbc_address =3D i915_gem_obj_ggtt_offset(obj); Ah yeah, pin_display is good. Not sure I like intel_fbc_active() much. But it needs to get replaced w/ some pipe config stuff or something anyway eventually. I do already check fbc.plane =3D=3D crtc->plane in intel_mark_fbc_dirty() which should catch the case when fbc is disabled anyway. > = > > + } > > + > > + if (fbc_obj) > > + fbc_address =3D i915_gem_obj_ggtt_offset(fbc_obj); > > + > > + /* need to nuke/cache_clean on IVB+? */ > > + ring->fbc_dirty =3D fbc_obj !=3D NULL; > > + > > + /* need to update FBC tracking? */ > > + ring->fbc_address_dirty =3D fbc_address !=3D ring->fbc_address; > = > I'm not going to mention the unused bits in fbc_address.... Just preparing myself for full 32bit addres space :) > = > > + ring->fbc_address =3D fbc_address; > > +} > > + > > +static void > > i915_gem_execbuffer_move_to_active(struct list_head *vmas, > > struct intel_ring_buffer *ring) > > { > > @@ -1150,6 +1179,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, vo= id *data, > > if (flags & I915_DISPATCH_SECURE && !batch_obj->has_global_gtt_mappin= g) > > i915_gem_gtt_bind_object(batch_obj, batch_obj->cache_level); > > = > > + i915_gem_execbuffer_mark_fbc_dirty(ring, &eb->vmas); > > + > > ret =3D i915_gem_execbuffer_move_to_gpu(ring, &eb->vmas); > > if (ret) > > goto err; > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i91= 5/intel_display.c > > index 12cf362..49c40aa 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -8064,6 +8064,32 @@ void intel_mark_idle(struct drm_device *dev) > > gen6_rps_idle(dev->dev_private); > > } > > = > > +void intel_mark_fbc_dirty(struct drm_i915_gem_object *obj, > > + struct intel_ring_buffer *ring, > > + struct drm_i915_gem_object **fbc_obj) > > +{ > > + struct drm_device *dev =3D obj->base.dev; > > + struct drm_i915_private *dev_priv =3D dev->dev_private; > > + struct drm_crtc *crtc; > > + > > + if (!i915_powersave) > > + return; > > + > > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > > + if (!crtc->fb) > > + continue; > > + > > + if (dev_priv->fbc.plane !=3D to_intel_crtc(crtc)->plane) > > + continue; > > + > > + if (to_intel_framebuffer(crtc->fb)->obj !=3D obj) > > + continue; > > + > > + WARN_ON(*fbc_obj && *fbc_obj !=3D obj); > > + *fbc_obj =3D obj; > > + } > > +} > > + > > void intel_mark_fb_busy(struct drm_i915_gem_object *obj, > > struct intel_ring_buffer *ring) > > { > > @@ -8081,8 +8107,6 @@ void intel_mark_fb_busy(struct drm_i915_gem_objec= t *obj, > > continue; > > = > > intel_increase_pllclock(crtc); > > - if (ring && intel_fbc_enabled(dev)) > > - ring->fbc_dirty =3D true; > > } > > } > > = > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/in= tel_drv.h > > index 6d701e7..5841f60 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -614,6 +614,9 @@ void intel_ddi_get_config(struct intel_encoder *enc= oder, > > /* intel_display.c */ > > int intel_pch_rawclk(struct drm_device *dev); > > void intel_mark_busy(struct drm_device *dev); > > +void intel_mark_fbc_dirty(struct drm_i915_gem_object *obj, > > + struct intel_ring_buffer *ring, > > + struct drm_i915_gem_object **fbc_obj); > > void intel_mark_fb_busy(struct drm_i915_gem_object *obj, > > struct intel_ring_buffer *ring); > > void intel_mark_idle(struct drm_device *dev); > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/int= el_pm.c > > index dc306e1..bfec348 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); > = > You can kill this write here, right? I'm killing the whole function later. This was a squash accident since I had Ben's earlier patches on the bottom originally. I'll drop this hunk. > = > = > > blt_ecoskpd |=3D GEN6_BLITTER_FBC_NOTIFY; > > I915_WRITE(GEN6_BLITTER_ECOSKPD, blt_ecoskpd); > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/= i915/intel_ringbuffer.c > > index 4649bf5..43f7eab 100644 > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > > @@ -207,6 +207,57 @@ intel_emit_post_sync_nonzero_flush(struct intel_ri= ng_buffer *ring) > > return 0; > > } > > = > > +static int gen6_blt_fbc_tracking(struct intel_ring_buffer *ring) > > +{ > > + int ret; > > + > > + if (!ring->fbc_address_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); > > + if (ring->fbc_address !=3D -1) > > + intel_ring_emit(ring, _MASKED_BIT_ENABLE(GEN6_BLITTER_FBC_NOTIFY)); > > + else > > + intel_ring_emit(ring, _MASKED_BIT_DISABLE(GEN6_BLITTER_FBC_NOTIFY)); > > + intel_ring_advance(ring); > > + > > + ring->fbc_address_dirty =3D true; > = > false, surely? Oops, yeah. And now I need to go and re-rest to make sure that didn't make it look like it works even when it doesn't. But it shouldn't since we recompute it from scratch for every execbuffer. -- = Ville Syrj=E4l=E4 Intel OTC