On Mon, 31 Jan 2011, Chris Wilson wrote:
> Hugh Dickins found that characters in xterm were going missing and oft
> delayed. Being the curious type, he managed to associate this with the
> new high-precision vblank patches; disabling these he found, restored
> the orderliness of his characters.
>
> The oddness begins when one realised that Hugh was not using vblanks at
> all on his system (fvwm and some xterms). Instead, all he had to go on
> were warning of a pipe underrun, curiously enough at around 60Hz. He
> poked and found that in addition to the underrun warning, the hardware
> was flagging the start of a new frame, a vblank, which in turn was
> kicking off the pending vblank processing code.
>
> There is little we can do for the underruns on Hugh's machine, a
> Crestline [965GM], which must have its FIFO watermarks set to 8.
> However, we do not need to process the vblank if we know that they are
> disabled...
>
> Reported-by: Hugh Dickins <[email protected]>
> Signed-off-by: Chris Wilson <[email protected]>
Thanks a lot for devising this, Chris, I really appreciate it.
But so far I can't be more enthusiastic than "seems to be a good
improvement most of the time" - though I've not spent long on this
laptop since putting it on to -rc3.
I've not seen it go wrong on x86_64, but my first boot with it on
i386 behaved just as badly as before; thereafter it's not gone wrong
yet (and I had to double-check boot.omsg to make sure I'd really been
running what I thought I was running the first time). I'm relying
upon it to type this mail, which I wouldn't have managed before.
As you know (I went off-list to send more info at the weekend), this
behaviour is very elusive, probably depends on more than one issue,
comes and goes with irrelevant patches and config changes and reboots.
So I don't yet have a lot of confidence that your patch will work out
better in the long run than what I tried at the weekend (when moving
do_intel_finish_page_flip's do_gettimeofday back into the work->event
block worked well until the evening). If I had time, I'd experiment
further - but I don't have time, so I'll just keep running with your
patch to gather more confidence in it.
So far, so (fairly) good.
Thanks,
Hugh
On Tue, 1 Feb 2011 09:34:12 -0800 (PST), Hugh Dickins <[email protected]> wrote:
> Thanks a lot for devising this, Chris, I really appreciate it.
>
> But so far I can't be more enthusiastic than "seems to be a good
> improvement most of the time" - though I've not spent long on this
> laptop since putting it on to -rc3.
Yeah, this is really just an interim patch. Removes one instance of doing
work when we don't need to, but it does not answer why the vblank patches
made your machines worse (since the same spurious interrupt bug is present
in 2.6.37 and earlier).
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
On Tue, 1 Feb 2011 09:34:12 -0800 (PST)
Hugh Dickins <[email protected]> wrote:
> On Mon, 31 Jan 2011, Chris Wilson wrote:
>
> > Hugh Dickins found that characters in xterm were going missing and oft
> > delayed. Being the curious type, he managed to associate this with the
> > new high-precision vblank patches; disabling these he found, restored
> > the orderliness of his characters.
> >
> > The oddness begins when one realised that Hugh was not using vblanks at
> > all on his system (fvwm and some xterms). Instead, all he had to go on
> > were warning of a pipe underrun, curiously enough at around 60Hz. He
> > poked and found that in addition to the underrun warning, the hardware
> > was flagging the start of a new frame, a vblank, which in turn was
> > kicking off the pending vblank processing code.
> >
> > There is little we can do for the underruns on Hugh's machine, a
> > Crestline [965GM], which must have its FIFO watermarks set to 8.
> > However, we do not need to process the vblank if we know that they are
> > disabled...
> >
> > Reported-by: Hugh Dickins <[email protected]>
> > Signed-off-by: Chris Wilson <[email protected]>
>
> Thanks a lot for devising this, Chris, I really appreciate it.
>
> But so far I can't be more enthusiastic than "seems to be a good
> improvement most of the time" - though I've not spent long on this
> laptop since putting it on to -rc3.
>
> I've not seen it go wrong on x86_64, but my first boot with it on
> i386 behaved just as badly as before; thereafter it's not gone wrong
> yet (and I had to double-check boot.omsg to make sure I'd really been
> running what I thought I was running the first time). I'm relying
> upon it to type this mail, which I wouldn't have managed before.
>
> As you know (I went off-list to send more info at the weekend), this
> behaviour is very elusive, probably depends on more than one issue,
> comes and goes with irrelevant patches and config changes and reboots.
Are you still seeing underruns during normal activity? I wonder if the
ones you were seeing before were only reported at 60Hz due to vblank
interrupt processing. If we failed to clear the underrun status, we'd
report one every time we got a vblank interrupt (since the underruns
don't report interrupts by themselves).
If so, that may just be a red herring in this case.
--
Jesse Barnes, Intel Open Source Technology Center
On Tue, 1 Feb 2011 09:46:43 -0800
Jesse Barnes <[email protected]> wrote:
> On Tue, 1 Feb 2011 09:34:12 -0800 (PST)
> Hugh Dickins <[email protected]> wrote:
> > As you know (I went off-list to send more info at the weekend), this
> > behaviour is very elusive, probably depends on more than one issue,
> > comes and goes with irrelevant patches and config changes and reboots.
>
> Are you still seeing underruns during normal activity? I wonder if the
> ones you were seeing before were only reported at 60Hz due to vblank
> interrupt processing. If we failed to clear the underrun status, we'd
> report one every time we got a vblank interrupt (since the underruns
> don't report interrupts by themselves).
>
> If so, that may just be a red herring in this case.
More random questions arise from the info provided:
- why are we ending up in the flip code at all? fvwm shouldn't
trigger that path...
- what's with all the underruns? it looks like we *do* ack those
flags as needed, so apparently they're valid, but they indicate a
serious problem with the display pipeline; maybe self-refresh
shouldn't be enabled on your system (that would increase memory
latency and potentially cause underruns), running with
i915.powersave=0 would disable that feature
The lack of text really does sound like a render cache flushing
problem, but the other issues are worrying as well, and could be
compounding things. And the last time I saw the issue, it was related
to compositing and required an X server fix. But supposedly you're not
using compositing, so...
--
Jesse Barnes, Intel Open Source Technology Center
On Tue, Feb 1, 2011 at 10:08 AM, Jesse Barnes <[email protected]> wrote:
> On Tue, 1 Feb 2011 09:46:43 -0800
> Jesse Barnes <[email protected]> wrote:
>>
>> Are you still seeing underruns during normal activity?
Yes. That is, I see the "pipe a underrun" messages when I set drm
debug 7: I'm unaware of any ill-effect from them, unless they are
indeed a factor in my unflushed text issue.
>> I wonder if the
>> ones you were seeing before were only reported at 60Hz due to vblank
>> interrupt processing. If we failed to clear the underrun status, we'd
>> report one every time we got a vblank interrupt (since the underruns
>> don't report interrupts by themselves).
I was surprised that i915_driver_irq_handler "Clear the PIPE(A|B)STAT
regs" writes back precisely the pipea_stats it reads in, I'd have
expected to clear something there (and did earlier experiment with
writing back 0: black screen at boot!). But assumed the protocol is
such that it acknowledges the status bits by writing same back.
>>
>> If so, that may just be a red herring in this case.
>
> More random questions arise from the info provided:
> - why are we ending up in the flip code at all? fvwm shouldn't
> trigger that path...
Right. I haven't double-checked the logic, but I believe it's because
of bits set in the underrunning pipea_stats. I did one time modify
the underrun message to print out pipea_stats, over five seconds most
(265) values were 0x80440207
(there were also 14 occurrences of 0x80440007, 5 of 0x80440004 and 3
of 0x80440204).
> - what's with all the underruns? it looks like we *do* ack those
> flags as needed, so apparently they're valid, but they indicate a
> serious problem with the display pipeline; maybe self-refresh
> shouldn't be enabled on your system (that would increase memory
> latency and potentially cause underruns), running with
> i915.powersave=0 would disable that feature
I just tried i915.powersave=0 but the underruns still appeared. I
then tried earlier kernels, and was surprised to find no underruns
with 2.6.34, 2.6.36: the underruns appeared with 2.6.37.
>
> The lack of text really does sound like a render cache flushing
> problem, but the other issues are worrying as well, and could be
> compounding things. And the last time I saw the issue, it was related
> to compositing and required an X server fix. But supposedly you're not
> using compositing, so...
That's right.
Hugh
On Tue, 1 Feb 2011 10:46:17 -0800
Hugh Dickins <[email protected]> wrote:
> On Tue, Feb 1, 2011 at 10:08 AM, Jesse Barnes <[email protected]> wrote:
> > On Tue, 1 Feb 2011 09:46:43 -0800
> > Jesse Barnes <[email protected]> wrote:
> >>
> >> Are you still seeing underruns during normal activity?
>
> Yes. That is, I see the "pipe a underrun" messages when I set drm
> debug 7: I'm unaware of any ill-effect from them, unless they are
> indeed a factor in my unflushed text issue.
They can cause your display to shake or flicker, or the memory
controller to get confused enough to hang your system.
> I was surprised that i915_driver_irq_handler "Clear the PIPE(A|B)STAT
> regs" writes back precisely the pipea_stats it reads in, I'd have
> expected to clear something there (and did earlier experiment with
> writing back 0: black screen at boot!). But assumed the protocol is
> such that it acknowledges the status bits by writing same back.
Exactly. High bits are the interrupt enable bits, low bits are set for
status, write of 1 to clear status.
> Right. I haven't double-checked the logic, but I believe it's because
> of bits set in the underrunning pipea_stats. I did one time modify
> the underrun message to print out pipea_stats, over five seconds most
> (265) values were 0x80440207
> (there were also 14 occurrences of 0x80440007, 5 of 0x80440004 and 3
> of 0x80440204).
Well, vblank interrupts are enabled (though they shouldn't be for your
config as far as we know), so the interrupt status and handling is
occurring as expected.
What I find strange is that you're seeing flip pending interrupts. Are
your symptoms affected if you remove the
I915_DISPLAY_PLANE_[AB]_FLIP_PENDING_INTERRUPT lines from
I915_INTERRUPT_ENABLE_FIX at the top of i915_irq.c?
Do you see any calls to drm_mode_page_flip_ioctl() in your environment?
> I just tried i915.powersave=0 but the underruns still appeared. I
> then tried earlier kernels, and was surprised to find no underruns
> with 2.6.34, 2.6.36: the underruns appeared with 2.6.37.
Ok, that rules out self-refresh then I think.
--
Jesse Barnes, Intel Open Source Technology Center
On Tue, Feb 1, 2011 at 11:32 AM, Jesse Barnes <[email protected]> wrote:
> What I find strange is that you're seeing flip pending interrupts. Are
> your symptoms affected if you remove the
> I915_DISPLAY_PLANE_[AB]_FLIP_PENDING_INTERRUPT lines from
> I915_INTERRUPT_ENABLE_FIX at the top of i915_irq.c?
No. The first time it looked like yes it fixed it; but when I
rebooted again, same symptoms and same underruns back: this thing is
very erratic.
> Do you see any calls to drm_mode_page_flip_ioctl() in your environment?
None at all.
Sometime I should do a bisection between v2.6.36 and v2.6.37 to see
what started the underruns; but I can't promise when "sometime" will
arrive.
Hugh
On Tue, 1 Feb 2011 19:37:09 -0800
Hugh Dickins <[email protected]> wrote:
> On Tue, Feb 1, 2011 at 11:32 AM, Jesse Barnes <[email protected]> wrote:
>
> > What I find strange is that you're seeing flip pending interrupts. Are
> > your symptoms affected if you remove the
> > I915_DISPLAY_PLANE_[AB]_FLIP_PENDING_INTERRUPT lines from
> > I915_INTERRUPT_ENABLE_FIX at the top of i915_irq.c?
>
> No. The first time it looked like yes it fixed it; but when I
> rebooted again, same symptoms and same underruns back: this thing is
> very erratic.
>
> > Do you see any calls to drm_mode_page_flip_ioctl() in your environment?
>
> None at all.
>
> Sometime I should do a bisection between v2.6.36 and v2.6.37 to see
> what started the underruns; but I can't promise when "sometime" will
> arrive.
But presumably the FLIP_PENDING_INTERRUPT bits are stuck on in your
case, otherwise we wouldn't be getting to flip prepare/finish at all.
Some updated docs indicate those bits may not be reliable on 965
(though earlier ones did), so we may need to disable the flip code
entirely on 965 if we can't figure out what's going wrong on your
config...
--
Jesse Barnes, Intel Open Source Technology Center
On Wed, 2 Feb 2011, Jesse Barnes wrote:
> On Tue, 1 Feb 2011 19:37:09 -0800
> Hugh Dickins <[email protected]> wrote:
>
> > On Tue, Feb 1, 2011 at 11:32 AM, Jesse Barnes <[email protected]> wrote:
> >
> > > What I find strange is that you're seeing flip pending interrupts. Are
> > > your symptoms affected if you remove the
> > > I915_DISPLAY_PLANE_[AB]_FLIP_PENDING_INTERRUPT lines from
> > > I915_INTERRUPT_ENABLE_FIX at the top of i915_irq.c?
> >
> > No. The first time it looked like yes it fixed it; but when I
> > rebooted again, same symptoms and same underruns back: this thing is
> > very erratic.
> >
> > > Do you see any calls to drm_mode_page_flip_ioctl() in your environment?
> >
> > None at all.
> >
> > Sometime I should do a bisection between v2.6.36 and v2.6.37 to see
> > what started the underruns; but I can't promise when "sometime" will
> > arrive.
>
> But presumably the FLIP_PENDING_INTERRUPT bits are stuck on in your
> case, otherwise we wouldn't be getting to flip prepare/finish at all.
>
> Some updated docs indicate those bits may not be reliable on 965
> (though earlier ones did), so we may need to disable the flip code
> entirely on 965 if we can't figure out what's going wrong on your
> config...
Sometime arrived, I did a bisection between v2.6.36 and v2.6.37,
and my "pipe a underrun"s on 965 begin with 29e1316ab129 drm/i915/tv:
Sleep before checking for state changes (appended below).
But that's a necessary patch for me: without it both VESA framebuffer
and X mistake the size and shape of the laptop screen, and my windows
don't fit in properly.
I experimented a little with intel_tv.c on 2.6.38-rc3 and rc4.
Indeed, just deleting that intel_wait_for_vblank() stops the underruns,
but leaves the display missized. Replacing it by msleep(20), as used to
be done, behaves the same as with intel_wait_for_vblank() there: underruns
with correctly sized display. "#if 0"ing all of intel_tv_detect_type(),
just returning -1 from it, gives no underruns and correctly sized display.
I was going to work with the latter, when my original unflushed text
problem resurfaced again (just as it had later done when trying Chris's
"Suppress spurious vblank interrupts" patch). It appears that the
underruns, while mysterious and worrisome, have litte or nothing to do
with the unflushed text problem which is making 2.6.38-rc unusable.
For the moment I've gone back to my patch moving intel_display.c's
do_gettimeofday() call into the block where it's needed; though that
too disappointed eventually before. It all rather stinks of something
uninitialized somewhere.
Hugh
commit 29e1316ab129f2d3a9ea874e7c9a4cb936f43542
Author: Chris Wilson <[email protected]>
Date: Wed Sep 22 19:10:09 2010 +0100
drm/i915/tv: Sleep before checking for state changes.
We need to wait for the PLLs to settle prior to detecting the state
changes. The BIOS writers guide suggests waiting for the next vblank.
Reported-by: Carlos R. Mafra <[email protected]>
Signed-off-by: Chris Wilson <[email protected]>
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 49ab11c..106560b 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1271,8 +1271,12 @@ intel_tv_detect_type (struct intel_tv *intel_tv)
I915_WRITE(TV_DAC, tv_dac);
POSTING_READ(TV_DAC);
+ intel_wait_for_vblank(intel_tv->base.base.dev,
+ to_intel_crtc(intel_tv->base.base.crtc)->pipe);
+
type = -1;
if (wait_for((tv_dac = I915_READ(TV_DAC)) & TVDAC_STATE_CHG, 20) == 0) {
+ DRM_DEBUG_KMS("TV detected: %x, %x\n", tv_ctl, tv_dac);
/*
* A B C
* 0 1 1 Composite
@@ -1289,8 +1293,7 @@ intel_tv_detect_type (struct intel_tv *intel_tv)
DRM_DEBUG_KMS("Detected Component TV connection\n");
type = DRM_MODE_CONNECTOR_Component;
} else {
- DRM_DEBUG_KMS("Unrecognised TV connection: %x\n",
- tv_dac);
+ DRM_DEBUG_KMS("Unrecognised TV connection\n");
}
}
The documentation recommends that we should use a polling method for TV
detection as this is more power efficient than the interrupt based
mechanism (as the encoder can be completely switched off). A secondary
effect is that leaving the hotplug enabled seems to be causing pipe
underruns as reported by Hugh Dickins on his Crestline.
Signed-off-by: Chris Wilson <[email protected]>
Cc: [email protected]
---
Hugh, does this prevent the persistent PIPE UNDERRUN issue?
---
drivers/gpu/drm/i915/intel_tv.c | 43 +++++++++++++++++++++++++++-----------
1 files changed, 30 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 93206e4..fe4a53a 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1234,7 +1234,8 @@ static const struct drm_display_mode reported_modes[] = {
* \return false if TV is disconnected.
*/
static int
-intel_tv_detect_type (struct intel_tv *intel_tv)
+intel_tv_detect_type (struct intel_tv *intel_tv,
+ struct drm_connector *connector)
{
struct drm_encoder *encoder = &intel_tv->base.base;
struct drm_device *dev = encoder->dev;
@@ -1245,11 +1246,13 @@ intel_tv_detect_type (struct intel_tv *intel_tv)
int type;
/* Disable TV interrupts around load detect or we'll recurse */
- spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
- i915_disable_pipestat(dev_priv, 0,
- PIPE_HOTPLUG_INTERRUPT_ENABLE |
- PIPE_HOTPLUG_TV_INTERRUPT_ENABLE);
- spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+ if (connector->polled & DRM_CONNECTOR_POLL_HPD) {
+ spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+ i915_disable_pipestat(dev_priv, 0,
+ PIPE_HOTPLUG_INTERRUPT_ENABLE |
+ PIPE_HOTPLUG_TV_INTERRUPT_ENABLE);
+ spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+ }
save_tv_dac = tv_dac = I915_READ(TV_DAC);
save_tv_ctl = tv_ctl = I915_READ(TV_CTL);
@@ -1302,11 +1305,13 @@ intel_tv_detect_type (struct intel_tv *intel_tv)
I915_WRITE(TV_CTL, save_tv_ctl);
/* Restore interrupt config */
- spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
- i915_enable_pipestat(dev_priv, 0,
- PIPE_HOTPLUG_INTERRUPT_ENABLE |
- PIPE_HOTPLUG_TV_INTERRUPT_ENABLE);
- spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+ if (connector->polled & DRM_CONNECTOR_POLL_HPD) {
+ spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+ i915_enable_pipestat(dev_priv, 0,
+ PIPE_HOTPLUG_INTERRUPT_ENABLE |
+ PIPE_HOTPLUG_TV_INTERRUPT_ENABLE);
+ spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+ }
return type;
}
@@ -1356,7 +1361,7 @@ intel_tv_detect(struct drm_connector *connector, bool force)
drm_mode_set_crtcinfo(&mode, CRTC_INTERLACE_HALVE_V);
if (intel_tv->base.base.crtc && intel_tv->base.base.crtc->enabled) {
- type = intel_tv_detect_type(intel_tv);
+ type = intel_tv_detect_type(intel_tv, connector);
} else if (force) {
struct drm_crtc *crtc;
int dpms_mode;
@@ -1364,7 +1369,7 @@ intel_tv_detect(struct drm_connector *connector, bool force)
crtc = intel_get_load_detect_pipe(&intel_tv->base, connector,
&mode, &dpms_mode);
if (crtc) {
- type = intel_tv_detect_type(intel_tv);
+ type = intel_tv_detect_type(intel_tv, connector);
intel_release_load_detect_pipe(&intel_tv->base, connector,
dpms_mode);
} else
@@ -1658,6 +1663,18 @@ intel_tv_init(struct drm_device *dev)
intel_encoder = &intel_tv->base;
connector = &intel_connector->base;
+ /* The documentation, for the older chipsets at least, recommend
+ * using a polling method rather than hotplug detection for TVs.
+ * This is because in order to perform the hotplug detection, the PLLs
+ * for the TV must be kept alive increasing power drain and starving
+ * bandwidth from other encoders. Notably for instance, it causes
+ * pipe underruns on Crestline when this encoder is supposedly idle.
+ *
+ * More recent chipsets favour HDMI rather than integrated S-Video.
+ */
+ connector->polled =
+ DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
+
drm_connector_init(dev, connector, &intel_tv_connector_funcs,
DRM_MODE_CONNECTOR_SVIDEO);
--
1.7.2.3
On Thu, 10 Feb 2011, Chris Wilson wrote:
> The documentation recommends that we should use a polling method for TV
> detection as this is more power efficient than the interrupt based
> mechanism (as the encoder can be completely switched off). A secondary
> effect is that leaving the hotplug enabled seems to be causing pipe
> underruns as reported by Hugh Dickins on his Crestline.
>
> Signed-off-by: Chris Wilson <[email protected]>
> Cc: [email protected]
> ---
>
> Hugh, does this prevent the persistent PIPE UNDERRUN issue?
Brilliant, Chris, yes it does, thank you. I checked both 2.6.38-rc4
and 2.6.37 (changing "irq_lock" back to "user_irq_lock") and verified
that both i386 and x86_64 "pipe a underrun"s have been fixed. I also
just tried hooking up laptop to TV by VGA cable, and it appears to
drive the TV correctly too.
(It doesn't fix my text flush problem, but we'd given up expecting that.)
Thanks,
Hugh
>
> ---
> drivers/gpu/drm/i915/intel_tv.c | 43 +++++++++++++++++++++++++++-----------
> 1 files changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index 93206e4..fe4a53a 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1234,7 +1234,8 @@ static const struct drm_display_mode reported_modes[] = {
> * \return false if TV is disconnected.
> */
> static int
> -intel_tv_detect_type (struct intel_tv *intel_tv)
> +intel_tv_detect_type (struct intel_tv *intel_tv,
> + struct drm_connector *connector)
> {
> struct drm_encoder *encoder = &intel_tv->base.base;
> struct drm_device *dev = encoder->dev;
> @@ -1245,11 +1246,13 @@ intel_tv_detect_type (struct intel_tv *intel_tv)
> int type;
>
> /* Disable TV interrupts around load detect or we'll recurse */
> - spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> - i915_disable_pipestat(dev_priv, 0,
> - PIPE_HOTPLUG_INTERRUPT_ENABLE |
> - PIPE_HOTPLUG_TV_INTERRUPT_ENABLE);
> - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> + if (connector->polled & DRM_CONNECTOR_POLL_HPD) {
> + spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> + i915_disable_pipestat(dev_priv, 0,
> + PIPE_HOTPLUG_INTERRUPT_ENABLE |
> + PIPE_HOTPLUG_TV_INTERRUPT_ENABLE);
> + spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> + }
>
> save_tv_dac = tv_dac = I915_READ(TV_DAC);
> save_tv_ctl = tv_ctl = I915_READ(TV_CTL);
> @@ -1302,11 +1305,13 @@ intel_tv_detect_type (struct intel_tv *intel_tv)
> I915_WRITE(TV_CTL, save_tv_ctl);
>
> /* Restore interrupt config */
> - spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> - i915_enable_pipestat(dev_priv, 0,
> - PIPE_HOTPLUG_INTERRUPT_ENABLE |
> - PIPE_HOTPLUG_TV_INTERRUPT_ENABLE);
> - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> + if (connector->polled & DRM_CONNECTOR_POLL_HPD) {
> + spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> + i915_enable_pipestat(dev_priv, 0,
> + PIPE_HOTPLUG_INTERRUPT_ENABLE |
> + PIPE_HOTPLUG_TV_INTERRUPT_ENABLE);
> + spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> + }
>
> return type;
> }
> @@ -1356,7 +1361,7 @@ intel_tv_detect(struct drm_connector *connector, bool force)
> drm_mode_set_crtcinfo(&mode, CRTC_INTERLACE_HALVE_V);
>
> if (intel_tv->base.base.crtc && intel_tv->base.base.crtc->enabled) {
> - type = intel_tv_detect_type(intel_tv);
> + type = intel_tv_detect_type(intel_tv, connector);
> } else if (force) {
> struct drm_crtc *crtc;
> int dpms_mode;
> @@ -1364,7 +1369,7 @@ intel_tv_detect(struct drm_connector *connector, bool force)
> crtc = intel_get_load_detect_pipe(&intel_tv->base, connector,
> &mode, &dpms_mode);
> if (crtc) {
> - type = intel_tv_detect_type(intel_tv);
> + type = intel_tv_detect_type(intel_tv, connector);
> intel_release_load_detect_pipe(&intel_tv->base, connector,
> dpms_mode);
> } else
> @@ -1658,6 +1663,18 @@ intel_tv_init(struct drm_device *dev)
> intel_encoder = &intel_tv->base;
> connector = &intel_connector->base;
>
> + /* The documentation, for the older chipsets at least, recommend
> + * using a polling method rather than hotplug detection for TVs.
> + * This is because in order to perform the hotplug detection, the PLLs
> + * for the TV must be kept alive increasing power drain and starving
> + * bandwidth from other encoders. Notably for instance, it causes
> + * pipe underruns on Crestline when this encoder is supposedly idle.
> + *
> + * More recent chipsets favour HDMI rather than integrated S-Video.
> + */
> + connector->polled =
> + DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
> +
> drm_connector_init(dev, connector, &intel_tv_connector_funcs,
> DRM_MODE_CONNECTOR_SVIDEO);
>
> --
> 1.7.2.3
On Feb 8, 2011, at 8:52 PM, Hugh Dickins wrote:
>>
>> But presumably the FLIP_PENDING_INTERRUPT bits are stuck on in your
>> case, otherwise we wouldn't be getting to flip prepare/finish at all.
>>
>> Some updated docs indicate those bits may not be reliable on 965
>> (though earlier ones did), so we may need to disable the flip code
>> entirely on 965 if we can't figure out what's going wrong on your
>> config...
>
> Sometime arrived, I did a bisection between v2.6.36 and v2.6.37,
> and my "pipe a underrun"s on 965 begin with 29e1316ab129 drm/i915/tv:
> Sleep before checking for state changes (appended below).
>
> But that's a necessary patch for me: without it both VESA framebuffer
> and X mistake the size and shape of the laptop screen, and my windows
> don't fit in properly.
>
> I experimented a little with intel_tv.c on 2.6.38-rc3 and rc4.
> Indeed, just deleting that intel_wait_for_vblank() stops the
> underruns,
> but leaves the display missized. Replacing it by msleep(20), as
> used to
> be done, behaves the same as with intel_wait_for_vblank() there:
> underruns
> with correctly sized display. "#if 0"ing all of
> intel_tv_detect_type(),
> just returning -1 from it, gives no underruns and correctly sized
> display.
> I was going to work with the latter, when my original unflushed text
> problem resurfaced again (just as it had later done when trying
> Chris's
> "Suppress spurious vblank interrupts" patch). It appears that the
> underruns, while mysterious and worrisome, have litte or nothing to do
> with the unflushed text problem which is making 2.6.38-rc unusable.
>
> For the moment I've gone back to my patch moving intel_display.c's
> do_gettimeofday() call into the block where it's needed; though that
> too disappointed eventually before. It all rather stinks of something
> uninitialized somewhere.
Just a remark, the do_gettimeofday() call is placed where it is
(Before the spin_lock_irqsave() call for the event_lock), so that
taking that timestamp (which is only needed later for some
comparisons) isn't delayed by a possible blocking on that lock.
Getting the timestamp as early as possible after entering that
function is needed to make pageflip timestamping more robust.
I'm puzzled why calling do_gettimeofday() could cause any harm, even
if that code gets executed by accident. I stared at the code for a
while and couldn't see missing initializations or similar. Maybe it
would still make sense to try to get some ftrace's on how the code
there executes, e.g., which path does it actually take or if some
piece of code takes an unusual and excessive amount of time to execute?
-mario
On Fri, Feb 11, 2011 at 10:21 AM, Mario Kleiner
<[email protected]> wrote:
> On Feb 8, 2011, at 8:52 PM, Hugh Dickins wrote:
>> It appears that the
>> underruns, while mysterious and worrisome, have litte or nothing to do
>> with the unflushed text problem which is making 2.6.38-rc unusable.
>>
>> For the moment I've gone back to my patch moving intel_display.c's
>> do_gettimeofday() call into the block where it's needed; though that
>> too disappointed eventually before. It all rather stinks of something
>> uninitialized somewhere.
>
> Just a remark, the do_gettimeofday() call is placed where it is (Before the
> spin_lock_irqsave() call for the event_lock), so that taking that timestamp
> (which is only needed later for some comparisons) isn't delayed by a
> possible blocking on that lock. Getting the timestamp as early as possible
> after entering that function is needed to make pageflip timestamping more
> robust.
>
> I'm puzzled why calling do_gettimeofday() could cause any harm, even if that
> code gets executed by accident. I stared at the code for a while and
> couldn't see missing initializations or similar. Maybe it would still make
> sense to try to get some ftrace's on how the code there executes, e.g.,
> which path does it actually take or if some piece of code takes an unusual
> and excessive amount of time to execute?
I'm sorry to say that I have now given up on this: it has already
consumed a lot more time than I can afford to give it. So I've now
just turned CONFIG_DRM_I915_KMS off, which gives me a laptop on which
2.6.38-rc is usable.
If in due course there's a likely patch someone would like me to try,
that I can do. And from time to time, with new kernels and with
upgraded userspace, I'll give I915_KMS a try again - indeed, for the
moment I still have it in my 64bit kernel.
Before switching KMS off, I did establish that, with Chris's overrun
fix, do_intel_finish_page_flip() - the function whose call to
do_gettimeofday() I moved - is no longer called at all. So that
modification was just cargo-cult magic by now (though perhaps made a
difference to timings when overruns were happening), and there's no
reason to suspect Mario's vblank patch at all. Let's assume that if I
attempted a fifth bisection, it would lead me to another (equally
blameless) patch. Nobody else is complaining: maybe my 965 is broken
and just gets along better with 2.6.38 KMS off.
Hugh
On Mon, 14 Feb 2011, Hugh Dickins wrote:
> On Fri, Feb 11, 2011 at 10:21 AM, Mario Kleiner
> <[email protected]> wrote:
> > On Feb 8, 2011, at 8:52 PM, Hugh Dickins wrote:
> >> It appears that the
> >> underruns, while mysterious and worrisome, have litte or nothing to do
> >> with the unflushed text problem which is making 2.6.38-rc unusable.
> >>
> I'm sorry to say that I have now given up on this: it has already
> consumed a lot more time than I can afford to give it. So I've now
> just turned CONFIG_DRM_I915_KMS off, which gives me a laptop on which
> 2.6.38-rc is usable.
>
> If in due course there's a likely patch someone would like me to try,
> that I can do. And from time to time, with new kernels and with
> upgraded userspace, I'll give I915_KMS a try again - indeed, for the
> moment I still have it in my 64bit kernel.
>
> Before switching KMS off, I did establish that, with Chris's overrun
> fix, do_intel_finish_page_flip() - the function whose call to
> do_gettimeofday() I moved - is no longer called at all. So that
> modification was just cargo-cult magic by now (though perhaps made a
> difference to timings when overruns were happening), and there's no
> reason to suspect Mario's vblank patch at all. Let's assume that if I
> attempted a fifth bisection, it would lead me to another (equally
> blameless) patch. Nobody else is complaining: maybe my 965 is broken
> and just gets along better with 2.6.38 KMS off.
Just wanted to report that something (sorry, no time to work out what)
in 3.0-rc1 fixed this issue for me, so now I am back on I915_KMS: thanks.
Hugh