drm/i915: enable trickle feed on Haswell

From: Paulo Zanoni <[email protected]>

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 <[email protected]>

Signed-off-by Paulo Zanoni <[email protected]>

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

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.

drivers/gpu/drm/i915/i915_reg.h +1 -0

index bbbc177..e92bb47 100644
@@ -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

drivers/gpu/drm/i915/intel_display.c +7 -3

index c64631d..d5f038e 100644
@@ -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;

drivers/gpu/drm/i915/intel_pm.c +0 -2

index 4605682..3a5c0bb 100644
@@ -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);
 

drivers/gpu/drm/i915/intel_sprite.c +5 -2

index 78b621c..ad6ec4b 100644
@@ -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))

Signature · 6 lines

1.8.1.2

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 8 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>

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.

Quoted 2 lines

Reported-by Arthur Runyan <[email protected]>

Signed-off-by Paulo Zanoni <[email protected]>

Reviewed-by Ville Syrjälä <[email protected]>

Quoted 8 lines

---
 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(-)

Quoted 6 lines

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

Quoted 5 lines

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 :)

Quoted 80 lines

drivers/gpu/drm/i915/i915_reg.h +1 -0

index bbbc177..e92bb47 100644
@@ -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

drivers/gpu/drm/i915/intel_display.c +7 -3

index c64631d..d5f038e 100644
@@ -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;

drivers/gpu/drm/i915/intel_pm.c +0 -2

index 4605682..3a5c0bb 100644
@@ -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);
 

drivers/gpu/drm/i915/intel_sprite.c +5 -2

index 78b621c..ad6ec4b 100644
@@ -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))

Signature · 6 lines

1.8.1.2

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Hi Daniel, hi folks,

just playing again with the support code for the NS2501 DVO in my old laptop. Despite finding one bug I'll send a patch
for soon, there is something else that makes we wonder, and that is the connection to external monitors.

Just to remind you, the NS2501 in this specific laptop, or probably in general, seems to be clocked by parts of the
sync signal of the display pipe. If the display pipe does not deliver the right clock, the DVO locks up and does not
react anymore to any commands on the i2c bus. The current ns2501 DVO driver code thus includes a little hack that,
if it detects that the DVO remains silent, forces the pipe A DLL to the right values to reactivate it. This is a hack, though
it works.

Now, interesting things happen if I connect an external monitor: The i915 code reads the edid data from the monitor,
finds that the monitor prefers a high clock rate (here an old CRT) of 75Hz vertical, and reconfigures the display pipe. Interestingly, this *also* seems to modify the display pipe of the DVO which should actually be connected to a
different pipe. Anyhow, as now the DVO sees a clock signal out of its operating range (60Hz vertical) it locks up and
no display appears *on the internal* LCD. In fact, the display breaks down completely and all you get is a residual
display on the TFT which no longer receives refresh. If I force the clocking of the external monitor to 60Hz, both
displays work again. So for me it looks like the role of the display pipes is somehow swapped if I connect an external
monitor - it seems as if in this case either both monitors are driven by the same pipe (why?) of that now pipe B feeds
the DVI1 hence the DVO and the TFT, and pipe A feeds the external monitor.

Thus, here my questions:
*) Can I, within the dvo driver code, somehow detect to which display pipe the DVO and thus the TFT is actually connected
to?

*) Can I somehow prohibit that the DVO is driven by *anything but* the 60Hz signal it likes, thus to prevent the lock-up
and disable the weird hack I'm currently using?

Somehow, the logic which display pipe drives which output is still unclear to me. Would be great if somebody could help
me out here.

Thanks,
Thomas

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 4 lines

Thus, here my questions:
*) Can I, within the dvo driver code, somehow detect to which display pipe the DVO and thus the TFT is actually connected
to?

Something like this:
intel_dvo = container_of(dvo, struct intel_dvo, dev); pipe = to_intel_ctrc(intel_dvo.base.base.crtc)->pipe

But currectly it looks like struct intel_dvo isn't visible outside intel_dvo.c which would need changing, or the hacks need to be moved into intel_dvoc.

Quoted 3 lines

*) Can I somehow prohibit that the DVO is driven by *anything but* the 60Hz signal it likes, thus to prevent the lock-up
and disable the weird hack I'm currently using?

I think currently it should be driven at the correct rate or not at all.

The CRT port can only be driven by pipe A, so I think that explains why your hacks may go bad when a CRT monitor is used.

I see several problems with the ns2501 code.
- it assumes DVOC. While that may be true always, getting the correct register from .dvo_reg is trivial

- assumes pipe A, which as stated isn't always true. During modesetting operations you can get the correct crtc via intel_dvo.base.base.crtc, from which you can get the pipe.

- the hack doesn't set up all the DPLL registers, but I suppose we could try to eliminate the hack. One thing that would need maybe fixing is the get_hw_state function. We could perhaps just change intel_dvo.c not to call the connector get_hw_state func if the encoder says the dvo port isn't active.

Quoted 3 lines

The CRT port can only be driven by pipe A, so I think that explains why your hacks may go bad when a CRT monitor is used.

Understood, this makes sense. Thus, DVO would then be at pipe B. How is this switch made, and where? There need to be a register somewhere that tells the i835 which signal goes where.

Quoted 8 lines · Thomas Richter

Quoted 3 lines

The CRT port can only be driven by pipe A, so I think that explains why your hacks may go bad when a CRT monitor is used.

Understood, this makes sense. Thus, DVO would then be at pipe B. How is this switch made, and where? There need to be a register somewhere that tells the i835 which signal goes where.

If the intel_connector->base.encoder pointer is non-NULL, then you can get at the pipe throught to_intel_crtc(intel_connector->base.encoder->crtc)->pipe. If this pointer doesn't exist then the dvo chip shouldn't be in use at the moment.

For intel_encoder you can chase to_intel_crtc(intel_encoder->base.crtc)->pipe, but you first need to check the crtc pointer for non-NULL.

btw you can check whether pipe B works without a 2nd display potentially wreaking havoc (so should be simpler to debug) with:

xrandr --output LVDS --crtc 1 --auto

Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

btw I've just read through your dvo code again and I think we can fix this easier. If I read your enable hack correctly then we need to have the dpll running and the DVO port on. The problem now is that in the dvo ->modeset callback this is explicitly _not_ the case.

This is not a big issue when there's only one pipe in use, but it wreaks havoc when more than one pipe is in use. So we need to move all that code somewhere else.

Now if you follow the callchains around the dvo->dpms callbacks the DVO port and DPLL are always enabled at that point in time, so I think we should be able to fix this all by moving the modeset code around to that place.

Quoted 4 lines

Now if you follow the callchains around the dvo->dpms callbacks the DVO port and DPLL are always enabled at that point in time, so I think we should be able to fix this all by moving the modeset code around to that place.

True, but probably with the high-speed bit in the wrong state.

Quoted 6 lines · Thomas Richter

Quoted 4 lines

Now if you follow the callchains around the dvo->dpms callbacks the DVO port and DPLL are always enabled at that point in time, so I think we should be able to fix this all by moving the modeset code around to that place.

Quoted 1 line

True, but probably with the high-speed bit in the wrong state.

We always program the dpll with the high-speed select bit set when using it for a DVO port, so this will also automatically be taken care of.

Actually, I was testing with the 3.10.10 kernel here. Unfortunately, I need to tell you that 3.11.4 and 3.12.0-rc.4 does *not work at all*. All I get on these kernel releases is a blank screen, though with a working mouse cursor. Apparently, nothing is rendered on the screen at all. Otherwise, it does the same as 3.10.10, namely lock up if I connect an external monitor during bootstrap.

For 3.12.0-rc.4, if I connect the monitor *after* bootup on the gdm login, I first see nothing on the external screen, and the mouse pointer on the internal TFT. Then, after logging in blindly, the cursor appears at the external monitor, the system beeps and then crashes and hangs.

Thus, sorry, but: broken in 3.11 and up...

Sorry for the bad news, please try try again.

I guess step one is to bisect the regression in 3.11. Fixing things on top of 3.10 is imo pointless if later kernels break things harder ...

Quoted 2 lines · Daniel Vetter

I guess step one is to bisect the regression in 3.11. Fixing things on top of 3.10 is imo pointless if later kernels break things harder ...

btw the two SNA patches I've just submitted should improve things quite a bit. If that gets rid of the black screen then there's not really a need to do the bisect.

Quoted 2 lines · Daniel Vetter

I guess step one is to bisect the regression in 3.11. Fixing things on top of 3.10 is imo pointless if later kernels break things harder ...

btw the two SNA patches I've just submitted should improve things quite a bit. If that gets rid of the black screen then there's not really a need to do the bisect.

That is, edit intel_display.c, function i8xx_update_dll(), and insert the following lines to setup the PLL value:

        if (inten_pipe_has_type(&crtc->base,INTEL_OUTPUT_DVO)) {
                dpll |= DPLL_DVO_HIGH_SPEED;
        }