On Fri, Aug 30, 2013 at 08:25:52PM +0300, Ville Syrjälä wrote:
> On Fri, Aug 23, 2013 at 07:51:28PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> >
> > We shouldn't disable the trickle feed bits on Haswell. Our
> > documentation explicitly says the trickle feed bits of PRI_CTL and
> > CUR_CTL should not be programmed to 1, and the hardware engineer also
> > asked us to not program the SPR_CTL field to 1. Leaving the bits as 1
> > could cause underflows.
> >
> > Reported-by: Arthur Runyan <arthur.j.runyan@xxxxxxxxx>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Queued for -next, thanks for the patch.
>
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 1 +
> > drivers/gpu/drm/i915/intel_display.c | 10 +++++++---
> > drivers/gpu/drm/i915/intel_pm.c | 2 --
> > drivers/gpu/drm/i915/intel_sprite.c | 7 +++++--
> > 4 files changed, 13 insertions(+), 7 deletions(-)
> >
> > Some side discussions:
> >
> > 1 - It seems we always do read-modify-write with the PRI/SPR/CUR registers. I
> > think this is dangerous since we may forget to disable something and keep our
> > code in a bugged state forever. Shouldn't we try to completely set the full
> > state of the bits from scratch every time we enable PRI/SPR/CUR?
>
> Yeah I dislike the RMW. In my atomic branch I think I got rid of it, if
> for no other reason than less overhead. I think the reason for the RMW
> was that the whole plane handling has been spread around all over the
> place. We should just centralize it nicely as part of the "all planes
> are drm_planes" change.
>
> The base address RMW is an open question though since it was explicitly
> made that way. But on HSW that's supposedly bugged and can revert the
> based address update from a command streamer flip. I haven't actually
> tried which way it's bugged: does it immediately revert and cause the
> display to scan out garbage until the new value is latches on the next
> vblank, or does the revert also need a vblank to latch. The latter
> option is the less dangerous since we'd need to get a vblank between
> the read and write to see garbage. I should write a test now that
> there's a hsw machine on my desk...
Yeah, the rmw is a bit uncool, and I've also forgotten what the exact
reason for merging this was - the commit message is especially unhelpful.
Imo if we need to preserve some bits in there we need to properly track
them. So I'm ok with reverting this again if it causes issues on Haswell
(or if it's just generally a mess to maintain ...).
-Daniel
> > 2 - We also have code to enable the trickle feed bits at init_clock_gating.
> > Isn't this dangerous? Trickle fee changes the memory is read, my first guess
> > would be that it's probably not safe to change this bit when things are
> > enabled. Also, we lose the state of these bits when the power well is disabled,
> > so the code in init_clock_gating really makes no sense on Haswell.
>
> I don't recall seeing any garbage when toggling trickle feed on the fly.
> I assume that if the watermarks are sufficiently high toggling trickle
> feed should be safe, well apparently before HSW that is :)
>
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index bbbc177..e92bb47 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3312,6 +3312,7 @@
> > #define MCURSOR_PIPE_A 0x00
> > #define MCURSOR_PIPE_B (1 << 28)
> > #define MCURSOR_GAMMA_ENABLE (1 << 26)
> > +#define CURSOR_TRICKLE_FEED_DISABLE (1 << 14)
> > #define _CURABASE (dev_priv->info->display_mmio_offset + 0x70084)
> > #define _CURAPOS (dev_priv->info->display_mmio_offset + 0x70088)
> > #define CURSOR_POS_MASK 0x007FF
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index c64631d..d5f038e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2077,8 +2077,10 @@ static int ironlake_update_plane(struct drm_crtc *crtc,
> > else
> > dspcntr &= ~DISPPLANE_TILED;
> >
> > - /* must disable */
> > - dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
> > + if (IS_HASWELL(dev))
> > + dspcntr &= ~DISPPLANE_TRICKLE_FEED_DISABLE;
> > + else
> > + dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
> >
> > I915_WRITE(reg, dspcntr);
> >
> > @@ -6768,8 +6770,10 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
> > cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
> > cntl |= CURSOR_MODE_DISABLE;
> > }
> > - if (IS_HASWELL(dev))
> > + if (IS_HASWELL(dev)) {
> > cntl |= CURSOR_PIPE_CSC_ENABLE;
> > + cntl &= ~CURSOR_TRICKLE_FEED_DISABLE;
> > + }
> > I915_WRITE(CURCNTR_IVB(pipe), cntl);
> >
> > intel_crtc->cursor_visible = visible;
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 4605682..3a5c0bb 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4950,8 +4950,6 @@ static void haswell_init_clock_gating(struct drm_device *dev)
> > I915_READ(GEN7_SQ_CHICKEN_MBCUNIT_CONFIG) |
> > GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB);
> >
> > - g4x_disable_trickle_feed(dev);
> > -
> > /* WaVSRefCountFullforceMissDisable:hsw */
> > gen7_setup_fixed_func_scheduler(dev_priv);
> >
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 78b621c..ad6ec4b 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -260,8 +260,11 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > if (obj->tiling_mode != I915_TILING_NONE)
> > sprctl |= SPRITE_TILED;
> >
> > - /* must disable */
> > - sprctl |= SPRITE_TRICKLE_FEED_DISABLE;
> > + if (IS_HASWELL(dev))
> > + sprctl &= ~SPRITE_TRICKLE_FEED_DISABLE;
> > + else
> > + sprctl |= SPRITE_TRICKLE_FEED_DISABLE;
> > +
> > sprctl |= SPRITE_ENABLE;
> >
> > if (IS_HASWELL(dev))
> > --
> > 1.8.1.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
