Hi
2013/3/28 Daniel Vetter <daniel at ffwll.ch>:
> On Fri, Feb 22, 2013 at 05:05:29PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> In this commit we enable both CPU and PCH FIFO underrun reporting and
>> start reporting them. We follow a few rules:
>> - after we receive one of these errors, we mask the interrupt, so
>> we won't get an "interrupt storm" and we also won't flood dmesg;
>> - at each mode set we enable the interrupts again, so we'll see each
>> message at most once per mode set;
>> - in the specific places where we need to ignore the errors, we
>> completely mask the interrupts.
>>
>> The downside of this patch is that since we're completely disabling
>> (masking) the interrupts instead of just not printing error messages,
>> we will mask more than just what we want on IVB/HSW CPU interrupts
>> (due to GEN7_ERR_INT) and on CPT/PPT/LPT PCHs (due to SERR_INT). So
>> when we decide to mask PCH FIFO underruns for pipe A on CPT, we'll
>> also be masking PCH FIFO underruns for pipe B, because both are
>> reported by SERR_INT which has to be either completely enabled or
>> completely disabled (in othe words, there's no way to disable/enable
>> specific bits of GEN7_ERR_INT and SERR_INT).
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>
> Ok, sorry for the long delay in reviewing this (I've somehow hoped that
> someone else would do it, but meh ...).
>
> I'm happy with the logic in your patch here, but it took me a while to
> piece together the control flow and what exactly is done where. I think
> that can be improved by renaming a few functions to make it clearer what
> exactly they're doing. Suggestions below.
Yeah, looks like the names were not really good. See below.
>
> The only functional comment is about the hsw err irq blocking, at least to
> me it looks not required.
>
> Yours, Daniel
>
>> ---
>> drivers/gpu/drm/i915/i915_irq.c | 307 +++++++++++++++++++++++++++++++++-
>> drivers/gpu/drm/i915/i915_reg.h | 13 +-
>> drivers/gpu/drm/i915/intel_display.c | 17 +-
>> drivers/gpu/drm/i915/intel_drv.h | 10 ++
>> 4 files changed, 334 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 7531095..d0f9c47 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -57,6 +57,208 @@ ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
>> }
>> }
>>
>> +static bool disable_gen7_err_int(struct drm_i915_private *dev_priv)
>
> It took me a while to figure out that this just checks whether underrun
> reporting is disabled on any crtc and whether we hence need to disable it
> completely. Usually I presume that a function call verb_something actually
> does what the verb say, e.g. disable_foo I expect that the function
> actually disables foo.
>
> Since I can't come up with a concise name for these two functions here
> (have_pipe_with_disabled_underrun_reporting is the best I could do ...)
> maybe just inline it into their respective (only) callsite and add a
> comment before the loop saying what it does? Maybe like
>
> bool can_enable_underrun_reporting = true;
>
> /* No per-crtc underrun irq disable bit, only a global one. */
> for_each_pipe {
> if (crtc->underrun_reporting_disabled)
> can_enable_underrun_reporting = false;
> }
>
> if (enable && can_enable_underrun_reporting)
> /* enable */
> else
> /* disable */
I renamed disable_gen7_err_int() to ivb_can_enable_err_int() (and
inverted its behavior, of course). Same thing with
cpt_can_enable_serr_int(). I hope it's understandable now :)
>> +{
>> + struct intel_crtc *crtc;
>> + enum pipe pipe;
>> +
>> + for_each_pipe(pipe) {
>> + crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>> +
>> + if (crtc->disable_cpu_fifo_underrun)
>
> Same here, I'd go with cpu_fifo_underrun_disabled to make it clear that
> it's a state tracking, not a request/action variable.
Done.
>
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static bool disable_serr_int(struct drm_i915_private *dev_priv)
>> +{
>> + enum pipe pipe;
>> + struct intel_crtc *crtc;
>> +
>> + for_each_pipe(pipe) {
>> + crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>> +
>> + if (crtc->disable_pch_fifo_underrun)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static void ironlake_report_fifo_underrun(struct drm_device *dev,
>> + enum pipe pipe, bool enable)
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + uint32_t bit = (pipe == PIPE_A) ? DE_PIPEA_FIFO_UNDERRUN :
>> + DE_PIPEB_FIFO_UNDERRUN;
>> +
>> + if (enable)
>> + ironlake_enable_display_irq(dev_priv, bit);
>> + else
>> + ironlake_disable_display_irq(dev_priv, bit);
>> +}
>> +
>> +static void ivybridge_report_fifo_underrun(struct drm_device *dev, bool enable)
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> + if (enable) {
>> + if (disable_gen7_err_int(dev_priv))
>> + return;
>> +
>> + I915_WRITE(GEN7_ERR_INT, ERR_INT_FIFO_UNDERRUN_A |
>> + ERR_INT_FIFO_UNDERRUN_B |
>> + ERR_INT_FIFO_UNDERRUN_C);
>> +
>> + ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
>> + } else {
>> + ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
>> + }
>> +}
>> +
>> +static void ibx_report_fifo_underrun(struct intel_crtc *crtc, bool enable)
>> +{
>> + struct drm_device *dev = crtc->base.dev;
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + uint32_t bit = (crtc->pipe == PIPE_A) ? SDE_TRANSA_FIFO_UNDER :
>> + SDE_TRANSB_FIFO_UNDER;
>> +
>> + if (enable)
>> + I915_WRITE(SDEIMR, I915_READ(SDEIMR) & ~bit);
>> + else
>> + I915_WRITE(SDEIMR, I915_READ(SDEIMR) | bit);
>> +
>> + POSTING_READ(SDEIMR);
>> +}
>> +
>> +static void cpt_report_fifo_underrun(struct drm_device *dev,
>> + enum transcoder pch_transcoder,
>> + bool enable)
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> + if (enable) {
>> + if (disable_serr_int(dev_priv))
>> + return;
>> +
>> + I915_WRITE(SERR_INT, SERR_INT_TRANS_A_FIFO_UNDERRUN |
>> + SERR_INT_TRANS_B_FIFO_UNDERRUN |
>> + SERR_INT_TRANS_C_FIFO_UNDERRUN);
>> +
>> + I915_WRITE(SDEIMR, I915_READ(SDEIMR) & ~SDE_ERROR_CPT);
>> + } else {
>> + I915_WRITE(SDEIMR, I915_READ(SDEIMR) | SDE_ERROR_CPT);
>> + }
>> +
>> + POSTING_READ(SDEIMR);
>> +}
>> +
>> +/**
>> + * intel_report_cpu_fifo_underrun - enable/disable FIFO underrun messages
>> + * @dev: drm device
>> + * @pipe: pipe
>> + * @enable: true if we want to report FIFO underrun errors, false otherwise
>> + *
>> + * This function makes us disable or report CPU fifo underruns for a specific
>
> "disable or enable" (instead of report) is imo clearer, see below.
Done.
>
>> + * pipe. Notice that on some Gens (e.g. IVB, HSW), disabling FIFO underrun
>> + * reporting for one pipe may also disable all the other CPU error interruts for
>> + * the other pipes, due to the fact that there's just one interrupt mask/enable
>> + * bit for all the pipes.
>> + *
>> + * Returns the previous state of underrun reporting.
>> + */
>> +bool intel_report_cpu_fifo_underrun(struct drm_device *dev, enum pipe pipe,
>> + bool enable)
>
> Again I've tripped up about the report verb, presuming that this function
> reports underruns. But it only updates the underrun reporting state, so
> maybe intel_*_fifo_underrun_reporting?
Changed to intel_set_{cpu,pch}_fifo_underrun_reporting.
>
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
>> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> + unsigned long flags;
>> + bool ret;
>> +
>> + spin_lock_irqsave(&dev_priv->irq_lock, flags);
>> +
>> + ret = !intel_crtc->disable_cpu_fifo_underrun;
>> +
>> + if (enable == ret)
>> + goto done;
>> +
>> + intel_crtc->disable_cpu_fifo_underrun = !enable;
>> +
>> + if (IS_GEN5(dev) || IS_GEN6(dev))
>> + ironlake_report_fifo_underrun(dev, pipe, enable);
>> + else if (IS_GEN7(dev))
>> + ivybridge_report_fifo_underrun(dev, enable);
>> +
>> +done:
>> + spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
>> + return ret;
>> +}
>> +
>> +/**
>> + * intel_report_pch_fifo_underrun - enable/disable FIFO underrun messages
>> + * @dev: drm device
>> + * @pch_transcoder: the PCH transcoder (same as pipe on IVB and older)
>> + * @enable: true if we want to report FIFO underrun errors, false otherwise
>> + *
>> + * This function makes us disable or report PCH fifo underruns for a specific
>> + * PCH transcoder. Notice that on some PCHs (e.g. CPT/PPT), disabling FIFO
>> + * underrun reporting for one transcoder may also disable all the other PCH
>> + * error interruts for the other transcoders, due to the fact that there's just
>> + * one interrupt mask/enable bit for all the transcoders.
>> + *
>> + * Returns the previous state of underrun reporting.
>> + */
>> +bool intel_report_pch_fifo_underrun(struct drm_device *dev,
>> + enum transcoder pch_transcoder,
>> + bool enable)
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + enum pipe p;
>> + struct drm_crtc *crtc;
>> + struct intel_crtc *intel_crtc;
>> + unsigned long flags;
>> + bool ret;
>> +
>> + if (HAS_PCH_LPT(dev)) {
>> + crtc = NULL;
>> + for_each_pipe(p) {
>> + struct drm_crtc *c = dev_priv->pipe_to_crtc_mapping[p];
>> + if (intel_pipe_has_type(c, INTEL_OUTPUT_ANALOG)) {
>> + crtc = c;
>> + break;
>> + }
>> + }
>> + if (!crtc) {
>> + DRM_ERROR("PCH FIFO underrun, but no CRTC using the PCH found\n");
>> + return false;
>> + }
>> + } else {
>> + crtc = dev_priv->pipe_to_crtc_mapping[pch_transcoder];
>> + }
>> + intel_crtc = to_intel_crtc(crtc);
>> +
>> + spin_lock_irqsave(&dev_priv->irq_lock, flags);
>> +
>> + ret = !intel_crtc->disable_pch_fifo_underrun;
>> +
>> + if (enable == ret)
>> + goto done;
>> +
>> + intel_crtc->disable_pch_fifo_underrun = !enable;
>> +
>> + if (HAS_PCH_IBX(dev))
>> + ibx_report_fifo_underrun(intel_crtc, enable);
>> + else
>> + cpt_report_fifo_underrun(dev, pch_transcoder, enable);
>> +
>> +done:
>> + spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
>> + return ret;
>> +}
>> +
>> void
>> i915_enable_pipestat(drm_i915_private_t *dev_priv, int pipe, u32 mask)
>> {
>> @@ -659,14 +861,57 @@ 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 (intel_report_pch_fifo_underrun(dev, TRANSCODER_A, false))
>> + DRM_ERROR("PCH transcoder A FIFO underrun\n");
>
> It sounds like we have outstanding issues with underruns still, but I'd
> like to merge your patch sooner than later (it looks like the kind with
> massive rebase pain). So keep things at DRM_DEBUG_DRIVER meanwhile, until
> we've fixed the know ones?
Done. Let's see what happens when we ship this :)
>
>> +
>> + if (pch_iir & SDE_TRANSB_FIFO_UNDER)
>> + if (intel_report_pch_fifo_underrun(dev, TRANSCODER_B, false))
>> + DRM_ERROR("PCH transcoder B FIFO underrun\n");
>> }
>>
>> +static void err_int_handler(struct drm_device *dev)
>
> snb_ prefix should be added imo.
Done. It's actually ivb_.
>
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + u32 err_int = I915_READ(GEN7_ERR_INT);
>> +
>> + if (err_int & ERR_INT_FIFO_UNDERRUN_A)
>> + if (intel_report_cpu_fifo_underrun(dev, PIPE_A, false))
>> + DRM_ERROR("Pipe A FIFO underrun\n");
>> +
>> + if (err_int & ERR_INT_FIFO_UNDERRUN_B)
>> + if (intel_report_cpu_fifo_underrun(dev, PIPE_B, false))
>> + DRM_ERROR("Pipe B FIFO underrun\n");
>> +
>> + if (err_int & ERR_INT_FIFO_UNDERRUN_C)
>> + if (intel_report_cpu_fifo_underrun(dev, PIPE_C, false))
>> + DRM_ERROR("Pipe C FIFO underrun\n");
>> +
>> + I915_WRITE(GEN7_ERR_INT, err_int);
>> +}
>> +
>> +static void serr_int_handler(struct drm_device *dev)
>
> Same here for an cpt_ prefix.
Done.
>
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + u32 serr_int = I915_READ(SERR_INT);
>> +
>> + if (serr_int & SERR_INT_TRANS_A_FIFO_UNDERRUN)
>> + if (intel_report_pch_fifo_underrun(dev, TRANSCODER_A, false))
>> + DRM_ERROR("PCH transcoder A FIFO underrun\n");
>> +
>> + if (serr_int & SERR_INT_TRANS_B_FIFO_UNDERRUN)
>> + if (intel_report_pch_fifo_underrun(dev, TRANSCODER_B, false))
>> + DRM_ERROR("PCH transcoder B FIFO underrun\n");
>> +
>> + if (serr_int & SERR_INT_TRANS_C_FIFO_UNDERRUN)
>> + if (intel_report_pch_fifo_underrun(dev, TRANSCODER_C, false))
>> + DRM_ERROR("PCH transcoder C FIFO underrun\n");
>> +
>> + I915_WRITE(SERR_INT, serr_int);
>> +}
>> static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
>> {
>> +
>> drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>> int pipe;
>>
>> @@ -695,6 +940,9 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
>> DRM_DEBUG_DRIVER(" pipe %c FDI IIR: 0x%08x\n",
>> pipe_name(pipe),
>> I915_READ(FDI_RX_IIR(pipe)));
>> +
>> + if (pch_iir & SDE_ERROR_CPT)
>> + serr_int_handler(dev);
>> }
>>
>> static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>> @@ -707,6 +955,14 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>>
>> atomic_inc(&dev_priv->irq_received);
>>
>> + /* We get interrupts on unclaimed registers, so check for this before we
>> + * do any I915_{READ,WRITE}. */
>> + if (IS_HASWELL(dev) &&
>> + (I915_READ_NOTRACE(FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) {
>> + DRM_ERROR("Unclaimed register before interrupt\n");
>> + I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
>> + }
>> +
>> /* disable master interrupt before clearing iir */
>> de_ier = I915_READ(DEIER);
>> I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
>> @@ -720,6 +976,12 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>> I915_WRITE(SDEIER, 0);
>> POSTING_READ(SDEIER);
>>
>> + /* On Haswell, also mask ERR_INT because we don't want to risk
>> + * generating "unclaimed register" interrupts from inside the interrupt
>> + * handler. */
>> + if (IS_HASWELL(dev))
>> + ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
>
> The above two chunks look like they've escaped from your unclaimed
> register series. Do we still need those?
Yes. We're enabling GEN7_ERR_INT on this patch, so now whenever we
poke an unclaimed register we'll get an interrupt. The good thing is
that this shouldn't be happening with our default configs :)
>
> Imo if we manage to create an unclaimed register interrupt in the irq
> handler, we deserve it. And the linux irq subsystem already ensures that a
> given irq handler can't run in parallel (if another irq fires while it's
> running, an immediate 2nd run is enqueued).
As far as I remember, depending on the case we may keep running irq
handlers forever. Also, we'll still print error messages about
unclaimed registers due to the FPGA_DBG register, so it's not like
errors will go unnoticed :)
I also added the comment suggested on your other email. Thanks for the review :)
<snip>
>
>> +
>> gt_iir = I915_READ(GTIIR);
>> if (gt_iir) {
>> snb_gt_irq_handler(dev, dev_priv, gt_iir);
>> @@ -729,6 +991,9 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>>
>> de_iir = I915_READ(DEIIR);
>> if (de_iir) {
>> + if (de_iir & DE_ERR_INT_IVB)
>> + err_int_handler(dev);
>> +
>> if (de_iir & DE_AUX_CHANNEL_A_IVB)
>> dp_aux_irq_handler(dev);
>>
>> @@ -766,6 +1031,9 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>> ret = IRQ_HANDLED;
>> }
>>
>> + if (IS_HASWELL(dev) && !disable_gen7_err_int(dev_priv))
>> + ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
>> +
>> I915_WRITE(DEIER, de_ier);
>> POSTING_READ(DEIER);
>> I915_WRITE(SDEIER, sde_ier);
>> @@ -833,6 +1101,14 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>> if (de_iir & DE_PIPEB_VBLANK)
>> drm_handle_vblank(dev, 1);
>>
>> + if (de_iir & DE_PIPEA_FIFO_UNDERRUN)
>> + if (intel_report_cpu_fifo_underrun(dev, PIPE_A, false))
>> + DRM_ERROR("Pipe A FIFO underrun\n");
>> +
>> + if (de_iir & DE_PIPEB_FIFO_UNDERRUN)
>> + if (intel_report_cpu_fifo_underrun(dev, PIPE_B, false))
>> + DRM_ERROR("Pipe B FIFO underrun\n");
>> +
>> if (de_iir & DE_PLANEA_FLIP_DONE) {
>> intel_prepare_page_flip(dev, 0);
>> intel_finish_page_flip_plane(dev, 0);
>> @@ -1966,14 +2242,20 @@ static void ibx_irq_postinstall(struct drm_device *dev)
>> drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>> u32 mask;
>>
>> - if (HAS_PCH_IBX(dev))
>> + if (HAS_PCH_IBX(dev)) {
>> mask = SDE_HOTPLUG_MASK |
>> SDE_GMBUS |
>> - SDE_AUX_MASK;
>> - else
>> + SDE_AUX_MASK |
>> + SDE_TRANSB_FIFO_UNDER |
>> + SDE_TRANSA_FIFO_UNDER;
>> + } else {
>> mask = SDE_HOTPLUG_MASK_CPT |
>> SDE_GMBUS_CPT |
>> - SDE_AUX_MASK_CPT;
>> + SDE_AUX_MASK_CPT |
>> + SDE_ERROR_CPT;
>> +
>> + I915_WRITE(SERR_INT, I915_READ(SERR_INT));
>> + }
>>
>> I915_WRITE(SDEIIR, I915_READ(SDEIIR));
>> I915_WRITE(SDEIMR, ~mask);
>> @@ -1989,7 +2271,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
>> /* enable kind of interrupts always enabled */
>> u32 display_mask = DE_MASTER_IRQ_CONTROL | DE_GSE | DE_PCH_EVENT |
>> DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE |
>> - DE_AUX_CHANNEL_A;
>> + DE_AUX_CHANNEL_A | DE_PIPEB_FIFO_UNDERRUN |
>> + DE_PIPEA_FIFO_UNDERRUN;
>> u32 render_irqs;
>>
>> dev_priv->irq_mask = ~display_mask;
>> @@ -2039,12 +2322,14 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>> DE_PLANEC_FLIP_DONE_IVB |
>> DE_PLANEB_FLIP_DONE_IVB |
>> DE_PLANEA_FLIP_DONE_IVB |
>> - DE_AUX_CHANNEL_A_IVB;
>> + DE_AUX_CHANNEL_A_IVB |
>> + DE_ERR_INT_IVB;
>> u32 render_irqs;
>>
>> dev_priv->irq_mask = ~display_mask;
>>
>> /* should always can generate irq */
>> + I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
>> I915_WRITE(DEIIR, I915_READ(DEIIR));
>> I915_WRITE(DEIMR, dev_priv->irq_mask);
>> I915_WRITE(DEIER,
>> @@ -2195,6 +2480,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>> I915_WRITE(DEIMR, 0xffffffff);
>> I915_WRITE(DEIER, 0x0);
>> I915_WRITE(DEIIR, I915_READ(DEIIR));
>> + if (IS_GEN7(dev))
>> + I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
>>
>> I915_WRITE(GTIMR, 0xffffffff);
>> I915_WRITE(GTIER, 0x0);
>> @@ -2203,6 +2490,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>> I915_WRITE(SDEIMR, 0xffffffff);
>> I915_WRITE(SDEIER, 0x0);
>> I915_WRITE(SDEIIR, I915_READ(SDEIIR));
>> + if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev))
>> + I915_WRITE(SERR_INT, I915_READ(SERR_INT));
>> }
>>
>> static void i8xx_irq_preinstall(struct drm_device * dev)
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index cd226c2..4d27320 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -520,7 +520,10 @@
>>
>> #define ERROR_GEN6 0x040a0
>> #define GEN7_ERR_INT 0x44040
>> -#define ERR_INT_MMIO_UNCLAIMED (1<<13)
>> +#define ERR_INT_MMIO_UNCLAIMED (1<<13)
>> +#define ERR_INT_FIFO_UNDERRUN_C (1<<6)
>> +#define ERR_INT_FIFO_UNDERRUN_B (1<<3)
>> +#define ERR_INT_FIFO_UNDERRUN_A (1<<0)
>>
>> #define FPGA_DBG 0x42300
>> #define FPGA_DBG_RM_NOCLAIM (1<<31)
>> @@ -3390,7 +3393,7 @@
>> #define DE_PIPEA_FIFO_UNDERRUN (1 << 0)
>>
>> /* More Ivybridge lolz */
>> -#define DE_ERR_DEBUG_IVB (1<<30)
>> +#define DE_ERR_INT_IVB (1<<30)
>> #define DE_GSE_IVB (1<<29)
>> #define DE_PCH_EVENT_IVB (1<<28)
>> #define DE_DP_A_HOTPLUG_IVB (1<<27)
>> @@ -3540,6 +3543,7 @@
>> SDE_PORTC_HOTPLUG_CPT | \
>> SDE_PORTB_HOTPLUG_CPT)
>> #define SDE_GMBUS_CPT (1 << 17)
>> +#define SDE_ERROR_CPT (1 << 16)
>> #define SDE_AUDIO_CP_REQ_C_CPT (1 << 10)
>> #define SDE_AUDIO_CP_CHG_C_CPT (1 << 9)
>> #define SDE_FDI_RXC_CPT (1 << 8)
>> @@ -3564,6 +3568,11 @@
>> #define SDEIIR 0xc4008
>> #define SDEIER 0xc400c
>>
>> +#define SERR_INT 0xc4040
>> +#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 */
>> #define PORTD_HOTPLUG_ENABLE (1 << 20)
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 9b0cd86..7152f35 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -41,7 +41,6 @@
>> #include <drm/drm_crtc_helper.h>
>> #include <linux/dma_remapping.h>
>>
>> -bool intel_pipe_has_type(struct drm_crtc *crtc, int type);
>> static void intel_increase_pllclock(struct drm_crtc *crtc);
>> static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
>>
>> @@ -3305,6 +3304,10 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>> return;
>>
>> intel_crtc->active = true;
>> +
>> + intel_report_cpu_fifo_underrun(dev, pipe, true);
>> + intel_report_pch_fifo_underrun(dev, pipe, true);
>> +
>> intel_update_watermarks(dev);
>>
>> if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
>> @@ -3397,10 +3400,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);
>>
>> + intel_report_cpu_fifo_underrun(dev, pipe, true);
>> + if (is_pch_port)
>> + intel_report_pch_fifo_underrun(dev, TRANSCODER_A, true);
>> +
>> + intel_update_watermarks(dev);
>> +
>> if (is_pch_port)
>> dev_priv->display.fdi_link_train(crtc);
>>
>> @@ -3484,6 +3492,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>> if (dev_priv->cfb_plane == plane)
>> intel_disable_fbc(dev);
>>
>> + intel_report_pch_fifo_underrun(dev, pipe, false);
>> intel_disable_pipe(dev_priv, pipe);
>>
>> /* Disable PF */
>> @@ -3497,6 +3506,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>> ironlake_fdi_disable(crtc);
>>
>> ironlake_disable_pch_transcoder(dev_priv, pipe);
>> + intel_report_pch_fifo_underrun(dev, pipe, true);
>>
>> if (HAS_PCH_CPT(dev)) {
>> /* disable TRANS_DP_CTL */
>> @@ -3566,6 +3576,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>> if (dev_priv->cfb_plane == plane)
>> intel_disable_fbc(dev);
>>
>> + if (is_pch_port)
>> + intel_report_pch_fifo_underrun(dev, TRANSCODER_A, false);
>> intel_disable_pipe(dev_priv, pipe);
>>
>> intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
>> @@ -3582,6 +3594,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>>
>> if (is_pch_port) {
>> lpt_disable_pch_transcoder(dev_priv);
>> + intel_report_pch_fifo_underrun(dev, TRANSCODER_A, true);
>> intel_ddi_fdi_disable(crtc);
>> }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 010e998..aa8f948 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -238,6 +238,9 @@ struct intel_crtc {
>>
>> /* reset counter value when the last flip was submitted */
>> unsigned int reset_counter;
>> +
>> + bool disable_cpu_fifo_underrun;
>> + bool disable_pch_fifo_underrun;
>> };
>>
>> struct intel_plane {
>> @@ -442,6 +445,7 @@ int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter);
>> extern void intel_attach_force_audio_property(struct drm_connector *connector);
>> extern void intel_attach_broadcast_rgb_property(struct drm_connector *connector);
>>
>> +extern bool intel_pipe_has_type(struct drm_crtc *crtc, int type);
>> extern void intel_crt_init(struct drm_device *dev);
>> extern void intel_hdmi_init(struct drm_device *dev,
>> int hdmi_reg, enum port port);
>> @@ -697,5 +701,11 @@ intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector);
>> extern void intel_ddi_fdi_disable(struct drm_crtc *crtc);
>>
>> extern void intel_display_handle_reset(struct drm_device *dev);
>> +extern bool intel_report_cpu_fifo_underrun(struct drm_device *dev,
>> + enum pipe pipe,
>> + bool enable);
>> +extern bool intel_report_pch_fifo_underrun(struct drm_device *dev,
>> + enum transcoder pch_transcoder,
>> + bool enable);
>>
>> #endif /* __INTEL_DRV_H__ */
>> --
>> 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
