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>
> ---
> 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...
> 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
