<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Apr 10, 2013 at 5:18 AM, Ville Syrjälä <span dir="ltr"><<a href="mailto:ville.syrjala@linux.intel.com" target="_blank">ville.syrjala@linux.intel.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, Apr 09, 2013 at 03:13:10PM -0300, Rodrigo Vivi wrote:<br>
> On Tue, Apr 9, 2013 at 5:35 AM, Ville Syrjälä <<a href="mailto:ville.syrjala@linux.intel.com">ville.syrjala@linux.intel.com</a><br>
> > wrote:<br>
><br>
> > On Mon, Apr 08, 2013 at 06:49:42PM -0300, Rodrigo Vivi wrote:<br>
> > > This patch introduce Frame Buffer Compression (FBC) support for HSW.<br>
> > > It adds a new function haswell_enable_fbc to avoid getting<br>
> > > ironlake_enable_fbc messed with many IS_HASWELL checks.<br>
> > ><br>
> > > Signed-off-by: Rodrigo Vivi <<a href="mailto:rodrigo.vivi@gmail.com">rodrigo.vivi@gmail.com</a>><br>
> > > ---<br>
> > >  drivers/gpu/drm/i915/i915_drv.c |  1 +<br>
> > >  drivers/gpu/drm/i915/i915_reg.h |  6 ++++++<br>
> > >  drivers/gpu/drm/i915/intel_pm.c | 44<br>
> > ++++++++++++++++++++++++++++++++++++++++-<br>
> > >  3 files changed, 50 insertions(+), 1 deletion(-)<br>
> > ><br>
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c<br>
> > b/drivers/gpu/drm/i915/i915_drv.c<br>
> > > index 0cfc778..88fd6fb 100644<br>
> > > --- a/drivers/gpu/drm/i915/i915_drv.c<br>
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c<br>
> > > @@ -291,6 +291,7 @@ static const struct intel_device_info<br>
> > intel_haswell_m_info = {<br>
> > >       GEN7_FEATURES,<br>
> > >       .is_haswell = 1,<br>
> > >       .is_mobile = 1,<br>
> > > +     .has_fbc = 1,<br>
> > >  };<br>
> > ><br>
> > >  static const struct pci_device_id pciidlist[] = {            /* aka */<br>
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h<br>
> > b/drivers/gpu/drm/i915/i915_reg.h<br>
> > > index 5e91fbb..cb8e213 100644<br>
> > > --- a/drivers/gpu/drm/i915/i915_reg.h<br>
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h<br>
> > > @@ -849,6 +849,12 @@<br>
> > >  #define   SNB_CPU_FENCE_ENABLE       (1<<29)<br>
> > >  #define DPFC_CPU_FENCE_OFFSET        0x100104<br>
> > ><br>
> > > +/* Framebuffer compression for Haswell */<br>
> > > +#define HSW_FBC_RT_BASE                      0x7020<br>
> > > +#define   HSW_FBC_RT_BASE_ADDR_SHIFT 12<br>
> > > +<br>
> > > +#define   HSW_DPFC_CTL_FENCE_EN              (1<<28)<br>
> > > +#define   HSW_DPFC_CTL_DISABLE_SLB_INIT      (1<<15)<br>
> > ><br>
> > >  /*<br>
> > >   * GPIO regs<br>
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c<br>
> > b/drivers/gpu/drm/i915/intel_pm.c<br>
> > > index 27f94cd..94e1c3a 100644<br>
> > > --- a/drivers/gpu/drm/i915/intel_pm.c<br>
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c<br>
> > > @@ -253,6 +253,43 @@ static bool ironlake_fbc_enabled(struct drm_device<br>
> > *dev)<br>
> > >       return I915_READ(ILK_DPFC_CONTROL) & DPFC_CTL_EN;<br>
> > >  }<br>
> > ><br>
> > > +static void haswell_enable_fbc(struct drm_crtc *crtc, unsigned long<br>
> > interval)<br>
> > > +{<br>
> > > +     struct drm_device *dev = crtc->dev;<br>
> > > +     struct drm_i915_private *dev_priv = dev->dev_private;<br>
> > > +     struct drm_framebuffer *fb = crtc->fb;<br>
> > > +     struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);<br>
> > > +     struct drm_i915_gem_object *obj = intel_fb->obj;<br>
> > > +     struct intel_crtc *intel_crtc = to_intel_crtc(crtc);<br>
> > > +     int plane = intel_crtc->plane == 0 ? DPFC_CTL_PLANEA :<br>
> > DPFC_CTL_PLANEB;<br>
> > > +     unsigned long stall_watermark = 200;<br>
> > > +     u32 dpfc_ctl;<br>
> > > +<br>
> > > +     dpfc_ctl = I915_READ(ILK_DPFC_CONTROL);<br>
> > > +     dpfc_ctl |= (plane | DPFC_CTL_LIMIT_1X);<br>
> ><br>
> > Accroding to BSpec FBC is always tied to plane A. Bit 30 is MBZ.<br>
> ><br>
><br>
> Yeah, you are right. I'm going to add a verification at  begining like:<br>
>      if (intel_crtc->plane != PLANE_A) {<br>
>     dev_priv->no_fbc_reason = FBC_BAD_PLANE;<br>
>     return;<br>
> }<br>
><br>
><br>
> > Maybe fix up plane C FBC support for IVB while you're poking at the<br>
> > general direction?<br>
> ><br>
><br>
> Actually I wasn't trying general directions since I splited it out. It was<br>
> just bad copy and paste actually.<br>
<br>
</div></div>I'm not a fan of copy pasting code and letting the old code paths rot.<br>
<div class="im"><br>
> ><br>
> > > +     dpfc_ctl |= (HSW_DPFC_CTL_FENCE_EN | obj->fence_reg);<br>
> ><br>
> > The CPU fence field must be written with 0. SNB/IVB could do with the<br>
> > same fix.<br>
> ><br>
><br>
> Where did you get this restriction for HSW?<br>
<br>
</div>BSpec.<br></blockquote><div><br></div><div style>Are you talking about bit 28 of 43208h DevHSW?</div><div style>I couldn't find this restriction anywhere.</div><div style>Besides that, setting it to 0 made me experience bugs like missing some small screen updates. I mean, when it is 0 I missed many "****" when typing my login password.</div>
<div style>When it is set FBC works fine.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> Should we write 0 or not touch<br>
> by removing lis line?<br>
<br>
</div>Not sure. The spec doesn't say whether the "CPU fence enable" bit still<br>
has some meaning or not. I guess the safe approach would be to set the<br>
fence to 0 and only set the fence enable bit when we actually have a<br>
fence.<br>
<br>
BTW the whole dpfc_ctl handing seems to a bit broken. You're doing it<br>
via RMW, but you're not clearing the fields that you modify. So you<br>
could be just ORing bits to whatever garbage the register had before.<br></blockquote><div><br></div><div style>Although I don't see how a garbage could end up there I completely agree with you. So I'm going to just set the register directly without using this temp variable.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
><br>
><br>
> Thanks for all your comments!<br>
><br>
><br>
> ><br>
> > > +     dpfc_ctl |= HSW_DPFC_CTL_DISABLE_SLB_INIT;<br>
> > > +     I915_WRITE(ILK_DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |<br>
> > > +                (stall_watermark << DPFC_RECOMP_STALL_WM_SHIFT) |<br>
> > > +                (interval << DPFC_RECOMP_TIMER_COUNT_SHIFT));<br>
> > > +     I915_WRITE(HSW_FBC_RT_BASE,<br>
> > > +                obj->gtt_offset << HSW_FBC_RT_BASE_ADDR_SHIFT |<br>
> > > +                ILK_FBC_RT_VALID);<br>
> > > +     /* enable it... */<br>
> > > +     I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);<br>
> > > +<br>
> > > +     if (obj->fence_reg != I915_FENCE_REG_NONE) {<br>
> > > +             I915_WRITE(SNB_DPFC_CTL_SA,<br>
> > > +                        SNB_CPU_FENCE_ENABLE | obj->fence_reg);<br>
> > > +             I915_WRITE(DPFC_CPU_FENCE_OFFSET, crtc->y);<br>
> > > +     } else<br>
> > > +             I915_WRITE(SNB_DPFC_CTL_SA, ~SNB_CPU_FENCE_ENABLE);<br>
> > > +<br>
> > > +     sandybridge_blit_fbc_update(dev);<br>
> > > +<br>
> > > +     DRM_DEBUG_KMS("enabled fbc on plane %d\n", intel_crtc->plane);<br>
> > > +}<br>
> > > +<br>
> > >  bool intel_fbc_enabled(struct drm_device *dev)<br>
> > >  {<br>
> > >       struct drm_i915_private *dev_priv = dev->dev_private;<br>
> > > @@ -4158,7 +4195,12 @@ void intel_init_pm(struct drm_device *dev)<br>
> > >       if (I915_HAS_FBC(dev)) {<br>
> > >               if (HAS_PCH_SPLIT(dev)) {<br>
> > >                       dev_priv->display.fbc_enabled =<br>
> > ironlake_fbc_enabled;<br>
> > > -                     dev_priv->display.enable_fbc = ironlake_enable_fbc;<br>
> > > +                     if (IS_HASWELL(dev))<br>
> > > +                             dev_priv->display.enable_fbc =<br>
> > > +                                     haswell_enable_fbc;<br>
> > > +                     else<br>
> > > +                             dev_priv->display.enable_fbc =<br>
> > > +                                     ironlake_enable_fbc;<br>
> > >                       dev_priv->display.disable_fbc =<br>
> > ironlake_disable_fbc;<br>
> > >               } else if (IS_GM45(dev)) {<br>
> > >                       dev_priv->display.fbc_enabled = g4x_fbc_enabled;<br>
> > > --<br>
> > > 1.8.1.4<br>
> > ><br>
> > > _______________________________________________<br>
> > > Intel-gfx mailing list<br>
> > > <a href="mailto:Intel-gfx@lists.freedesktop.org">Intel-gfx@lists.freedesktop.org</a><br>
> > > <a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx" target="_blank">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a><br>
> ><br>
> > --<br>
> > Ville Syrjälä<br>
> > Intel OTC<br>
> ><br>
><br>
><br>
><br>
> --<br>
> Rodrigo Vivi<br>
> Blog: <a href="http://blog.vivi.eng.br" target="_blank">http://blog.vivi.eng.br</a><br>
<br>
--<br>
Ville Syrjälä<br>
Intel OTC<br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div>Rodrigo Vivi</div><div>Blog: <a href="http://blog.vivi.eng.br" target="_blank">http://blog.vivi.eng.br</a></div><div> </div>
</div></div>