Hi
2013/2/14 Daniel Vetter <daniel at ffwll.ch>:
> On Thu, Feb 14, 2013 at 06:53:42PM -0200, Paulo Zanoni wrote:
>> Hi
>>
>> 2013/2/9 Daniel Vetter <daniel at ffwll.ch>:
>> > On Fri, Feb 08, 2013 at 05:35:19PM -0200, Paulo Zanoni wrote:
>> >> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> >>
>> >> Also add an "ignore" bit that avoids printing the message in two
>> >> cases:
>> >> - When the message is in fact expected.
>> >> - After we get the first message. In tihs case, we expect to get
>> >> hundreds of consecutive messages, so we just ignore all the
>> >> subsequent messages until the next crtc_enable.
>> >>
>> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> >
>> > Ah, here's the "block underrun reporting stuff". You need to set up this
>> > infrastructure first, then enable the error interrupts. Otherwise a poor
>> > bloke will hit the irq strom in between these two patches in a bisect and
>> > die from a heart attack when looking at dmesg.
>>
>> Nope. This patch is also the patch that adds the dmesg message, so
>> without this patch there is no message to ignore. Without this patch
>> we still get the interrupts, but this shouldn't be a problem because
>> we don't print anything and because of patch 5.
>>
>> >
>> > Also, you update the ignore_bla from the irq handler, so this needs proper
>> > locking.
>> Will fix.
>>
>> > And I vote for the bikeshed to be moved into struct intel_crtc,
>> > e.g.
>>
>> It may be a little slightly slower, since we'll have to look for the
>> crtc pointers on our structs. And on Haswell, checking which crtc is
>> using the PCH transcoder will be more complicated. I'll try to think
>> on a good solution.
>
> Imo speed doesn't matter at all - we shouldn't ever get these in the
> interrupt handler under normal conditions.
Nope. We get hundreds of interrupts at every crtc_disable. We are
*ignoring* them (by not printing anything), but we're still receiving
the interrupts (so we run the irq handler every time).
> And if we get too many of those
> while modesetting, so that we suck away cpu time, it's probably best to
> just disable all underrun reporting while doing a modeset.
With the current code I didn't notice and slowdowns, but didn't really
measure anything. Do you mean "disable all underrun reporting" (aka
don't print messages but still get the interrupts) or do you mean
"disable all the interrupts" (aka don't run the irq handler)?
The problem with disabling the interrupts is that you can't just
disable the "PCH FIFO underrun interrupts". You have to disable all
the SERR interrupts, so you may lose other possible error messages.
Same thing with the CPU on IVB/HSW.
>
>> My initial idea was to actually group all the "interrupt-related" bits
>> from dev_priv into an "interrupt struct", and put the ignore_fifo bits
>> there.
>>
>> >
>> > struct intel_crtc {
>> > ...
>> >
>> > struct spinlock underrun_lock;
>> > int underrun_reporting_enabled;
>> > }
>> >
>> > Then smash a little helper into every crtc_enable/disable function. Aside:
>> > you need to use the spin_lock_irqsafe/restore functions in the irq handler
>> > (and for safety also in the helpers). Another curious question: Why do we
>> > need separate ignore bits for the cpu and the pch?
>>
>> Because: (i) sometimes we get errors from one but not from the other;
>> (ii) during "pipe/transcoder disable" we only want to ignore the PCH
>> underruns, not the CPU underruns; (iii) on Haswell, there's no 1:1
>> mapping between CPU pipe and PCH transcoder.
>
> Ok, I see your aim with separate underrun reporting disables for pch/cpu.
> But I also fear that we'll have tons of fun with just enabling the
> underrun interrupts once the pipe is up, so a fine-grained report
> disabling scheme sounds a bit like overkill. At least until we've fixed
> the first set of big bugs.
What do you mean by "tons of fun"? As soon as we get the error
interrupt again we enable the "ignore" bit, so we'll see at most 1
message per mode set per pipe/transcoder.
>
> The other thing is that I think we should just disable the interrupts when
> we expect them (modeset) to avoid blowing through too much cpu time. So
> having just one knob for everything sounds simpler.
You mean definitely *disable* the interrupts? We can't just disable
the PCH FIFO underrun interrupts, we need to completely disable SERR,
which in turn disable all the other possible error messages (e.g.,
Poison, the other FIFO underruns from all the other pipes and also the
other error interrupts we're currently ignoring). Same thing with the
CPU FIFO underruns on IVB/Haswell: we need to disable GEN7_ERR_INT, so
we disable all the other interrupts it can generate.
>
> Wrt Haswell pch interrupts: Either loop through the crtc list and check
> ->pch_encoder or check the DDI E port. The former shouldn't be a locking
> issue as long as you grab the right irqsafe spinlocks first and check
> whether reporting is enabled, since locks serve as barriers, too.
>
> The reason for all this "can we keep this generic" bikeshed is that I'd
> like to enable underrun reporting for i9xx/vlv, too. So having a bit of
> abstraction would help.
I don't understand how the current code can't be used on i9xx/vlv too.
But based on this discussion, I think that instead of adding variables
that just ignore the interrupts (avoid printing the error messages
while still keeping the interrupts enabled) you want me to actually
completely disable the error interrupts as soon as we get the first
(this way we'll be forced to disable *all* the error interrupts on
*all* the pipes or transcoders). We could do this without adding
anything to dev_priv or intel_crtc, but the downside is that we'll be
blocking *all* the error messages at every crtc_disable. I can provide
a patch series for this, but it's not as "complete" as the current
series.
> -Daniel
>
>>
>> > -Daniel
>> >
>> >> ---
>> >> drivers/gpu/drm/i915/i915_drv.h | 3 +++
>> >> drivers/gpu/drm/i915/i915_irq.c | 37 +++++++++++++++++++++++++++++-----
>> >> drivers/gpu/drm/i915/i915_reg.h | 7 +++++--
>> >> drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++-
>> >> 4 files changed, 53 insertions(+), 8 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> >> index 08c5def..e96d75e 100644
>> >> --- a/drivers/gpu/drm/i915/i915_drv.h
>> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> >> @@ -909,6 +909,9 @@ typedef struct drm_i915_private {
>> >> struct work_struct hotplug_work;
>> >> bool enable_hotplug_processing;
>> >>
>> >> + /* Bit 0: PCH transcoder A and so on. */
>> >> + u8 ignore_pch_fifo_underrun;
>> >> +
>> >> int num_pipe;
>> >> int num_pch_pll;
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> >> index 703a08a..7497589 100644
>> >> --- a/drivers/gpu/drm/i915/i915_irq.c
>> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> >> @@ -659,10 +659,17 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
>> >> if (pch_iir & (SDE_TRANSB_CRC_ERR | SDE_TRANSA_CRC_ERR))
>> >> DRM_DEBUG_DRIVER("PCH transcoder CRC error interrupt\n");
>> >>
>> >> - if (pch_iir & SDE_TRANSB_FIFO_UNDER)
>> >> - DRM_DEBUG_DRIVER("PCH transcoder B underrun interrupt\n");
>> >> - if (pch_iir & SDE_TRANSA_FIFO_UNDER)
>> >> - DRM_DEBUG_DRIVER("PCH transcoder A underrun interrupt\n");
>> >> + if ((pch_iir & SDE_TRANSB_FIFO_UNDER) &&
>> >> + !(dev_priv->ignore_pch_fifo_underrun & (1 << TRANSCODER_B))) {
>> >> + DRM_DEBUG_DRIVER("PCH transcoder B underrun\n");
>> >> + dev_priv->ignore_pch_fifo_underrun |= (1 << TRANSCODER_B);
>> >> + }
>> >> +
>> >> + if ((pch_iir & SDE_TRANSA_FIFO_UNDER) &&
>> >> + !(dev_priv->ignore_pch_fifo_underrun & (1 << TRANSCODER_A))) {
>> >> + DRM_DEBUG_DRIVER("PCH transcoder A underrun\n");
>> >> + dev_priv->ignore_pch_fifo_underrun |= (1 << TRANSCODER_A);
>> >> + }
>> >> }
>> >>
>> >> static void err_int_handler(struct drm_device *dev)
>> >> @@ -684,6 +691,24 @@ static void serr_int_handler(struct drm_device *dev)
>> >> if (serr_int & SERR_INT_POISON)
>> >> DRM_ERROR("PCH poison interrupt\n");
>> >>
>> >> + if ((serr_int & SERR_INT_TRANS_A_FIFO_UNDERRUN) &&
>> >> + !(dev_priv->ignore_pch_fifo_underrun & (1 << TRANSCODER_A))) {
>> >> + DRM_ERROR("PCH transcoder A FIFO underrun\n");
>> >> + dev_priv->ignore_pch_fifo_underrun |= (1 << TRANSCODER_A);
>> >> + }
>> >> +
>> >> + if ((serr_int & SERR_INT_TRANS_B_FIFO_UNDERRUN) &&
>> >> + !(dev_priv->ignore_pch_fifo_underrun & (1 << TRANSCODER_B))) {
>> >> + DRM_ERROR("PCH transcoder B FIFO underrun\n");
>> >> + dev_priv->ignore_pch_fifo_underrun |= (1 << TRANSCODER_B);
>> >> + }
>> >> +
>> >> + if ((serr_int & SERR_INT_TRANS_C_FIFO_UNDERRUN) &&
>> >> + !(dev_priv->ignore_pch_fifo_underrun & (1 << TRANSCODER_C))) {
>> >> + DRM_ERROR("PCH transcoder C FIFO underrun\n");
>> >> + dev_priv->ignore_pch_fifo_underrun |= (1 << TRANSCODER_C);
>> >> + }
>> >> +
>> >> I915_WRITE(SERR_INT, serr_int);
>> >> }
>> >>
>> >> @@ -2000,7 +2025,9 @@ static void ibx_irq_postinstall(struct drm_device *dev)
>> >> mask = SDE_HOTPLUG_MASK |
>> >> SDE_GMBUS |
>> >> SDE_AUX_MASK |
>> >> - SDE_POISON;
>> >> + SDE_POISON |
>> >> + SDE_TRANSB_FIFO_UNDER |
>> >> + SDE_TRANSA_FIFO_UNDER;
>> >> } else {
>> >> mask = SDE_HOTPLUG_MASK_CPT |
>> >> SDE_GMBUS_CPT |
>> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> >> index f22e27d..d565bd7 100644
>> >> --- a/drivers/gpu/drm/i915/i915_reg.h
>> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> >> @@ -3554,8 +3554,11 @@
>> >> #define SDEIIR 0xc4008
>> >> #define SDEIER 0xc400c
>> >>
>> >> -#define SERR_INT 0xc4040
>> >> -#define SERR_INT_POISON (1 << 31)
>> >> +#define SERR_INT 0xc4040
>> >> +#define SERR_INT_POISON (1 << 31)
>> >> +#define SERR_INT_TRANS_C_FIFO_UNDERRUN (1 << 6)
>> >> +#define SERR_INT_TRANS_B_FIFO_UNDERRUN (1 << 3)
>> >> +#define SERR_INT_TRANS_A_FIFO_UNDERRUN (1 << 0)
>> >>
>> >> /* digital port hotplug */
>> >> #define PCH_PORT_HOTPLUG 0xc4030 /* SHOTPLUG_CTL */
>> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> >> index d75c6a0..67bfb58 100644
>> >> --- a/drivers/gpu/drm/i915/intel_display.c
>> >> +++ b/drivers/gpu/drm/i915/intel_display.c
>> >> @@ -3274,6 +3274,9 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>> >> return;
>> >>
>> >> intel_crtc->active = true;
>> >> +
>> >> + dev_priv->ignore_pch_fifo_underrun &= ~(1 << pipe);
>> >> +
>> >> intel_update_watermarks(dev);
>> >>
>> >> if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
>> >> @@ -3366,11 +3369,15 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>> >> return;
>> >>
>> >> intel_crtc->active = true;
>> >> - intel_update_watermarks(dev);
>> >>
>> >> is_pch_port = haswell_crtc_driving_pch(crtc);
>> >>
>> >> if (is_pch_port)
>> >> + dev_priv->ignore_pch_fifo_underrun &= ~(1 << TRANSCODER_A);
>> >> +
>> >> + intel_update_watermarks(dev);
>> >> +
>> >> + if (is_pch_port)
>> >> dev_priv->display.fdi_link_train(crtc);
>> >>
>> >> for_each_encoder_on_crtc(dev, crtc, encoder)
>> >> @@ -3453,6 +3460,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>> >> if (dev_priv->cfb_plane == plane)
>> >> intel_disable_fbc(dev);
>> >>
>> >> + dev_priv->ignore_pch_fifo_underrun |= (1 << pipe);
>> >> intel_disable_pipe(dev_priv, pipe);
>> >>
>> >> /* Disable PF */
>> >> @@ -3466,6 +3474,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>> >> ironlake_fdi_disable(crtc);
>> >>
>> >> ironlake_disable_pch_transcoder(dev_priv, pipe);
>> >> + dev_priv->ignore_pch_fifo_underrun &= ~(1 << pipe);
>> >>
>> >> if (HAS_PCH_CPT(dev)) {
>> >> /* disable TRANS_DP_CTL */
>> >> @@ -3535,6 +3544,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>> >> if (dev_priv->cfb_plane == plane)
>> >> intel_disable_fbc(dev);
>> >>
>> >> + if (is_pch_port)
>> >> + dev_priv->ignore_pch_fifo_underrun |= (1 << TRANSCODER_A);
>> >> intel_disable_pipe(dev_priv, pipe);
>> >>
>> >> intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
>> >> @@ -3551,6 +3562,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>> >>
>> >> if (is_pch_port) {
>> >> lpt_disable_pch_transcoder(dev_priv);
>> >> + dev_priv->ignore_pch_fifo_underrun &= ~(1 << TRANSCODER_A);
>> >> intel_ddi_fdi_disable(crtc);
>> >> }
>> >>
>> >> --
>> >> 1.7.10.4
>> >>
>> >> _______________________________________________
>> >> Intel-gfx mailing list
>> >> Intel-gfx at lists.freedesktop.org
>> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>> > --
>> > Daniel Vetter
>> > Software Engineer, Intel Corporation
>> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>>
>>
>>
>> --
>> Paulo Zanoni
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Paulo Zanoni
