[Intel-gfx] [PATCH] Don't touch the pipestat regs for detecting FIFO underrun. The kernel owns them.

[Jesse Barnes]

On Monday, December 29, 2008 1:45 pm Eric Anholt wrote:
> Since we don't perform any synchronization with the kernel on these regs,
> we could race with the kernel to write stale values and end up not having
> vblank interrupts enabled when somebody was waiting on one.
> ---
>  src/i830_display.c |    4 ----
>  src/i830_driver.c  |   26 --------------------------
>  2 files changed, 0 insertions(+), 30 deletions(-)
>
> diff --git a/src/i830_display.c b/src/i830_display.c
> index 2e5d55a..7a9999a 100644
> --- a/src/i830_display.c
> +++ b/src/i830_display.c
> @@ -1202,7 +1202,6 @@ i830_crtc_mode_set(xf86CrtcPtr crtc, DisplayModePtr
> mode, int dspstride_reg = (plane == 0) ? DSPASTRIDE : DSPBSTRIDE;
>      int dsppos_reg = (plane == 0) ? DSPAPOS : DSPBPOS;
>      int dspsize_reg = (plane == 0) ? DSPASIZE : DSPBSIZE;
> -    int pipestat_reg = (pipe == 0) ? PIPEASTAT : PIPEBSTAT;
>      int i, num_outputs = 0;
>      int refclk;
>      intel_clock_t clock;
> @@ -1514,9 +1513,6 @@ i830_crtc_mode_set(xf86CrtcPtr crtc, DisplayModePtr
> mode, #endif
>
>      i830WaitForVblank(pScrn);
> -
> -    /* Clear any FIFO underrun status that may have occurred normally */
> -    OUTREG(pipestat_reg, INREG(pipestat_reg) | FIFO_UNDERRUN);
>  }
>
>
> diff --git a/src/i830_driver.c b/src/i830_driver.c
> index 16ddc41..27d8694 100644
> --- a/src/i830_driver.c
> +++ b/src/i830_driver.c
> @@ -2495,10 +2495,6 @@ RestoreHWState(ScrnInfoPtr pScrn)
>         OUTREG(FBC_CONTROL, pI830->saveFBC_CONTROL);
>     }
>
> -   /* Clear any FIFO underrun status that may have occurred normally */
> -   OUTREG(PIPEASTAT, INREG(PIPEASTAT) | FIFO_UNDERRUN);
> -   OUTREG(PIPEBSTAT, INREG(PIPEBSTAT) | FIFO_UNDERRUN);
> -
>     vgaHWRestore(pScrn, vgaReg, VGA_SR_FONTS);
>     vgaHWLock(hwp);
>
> @@ -2583,7 +2579,6 @@ void
>  IntelEmitInvarientState(ScrnInfoPtr pScrn)
>  {
>     I830Ptr pI830 = I830PTR(pScrn);
> -   uint32_t ctx_addr;
>
>     if (pI830->accel == ACCEL_NONE)
>        return;
> @@ -2620,7 +2615,6 @@ I830BlockHandler(int i,
>      ScreenPtr pScreen = screenInfo.screens[i];
>      ScrnInfoPtr pScrn = xf86Screens[i];
>      I830Ptr pI830 = I830PTR(pScrn);
> -    xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(pScrn);
>
>      pScreen->BlockHandler = pI830->BlockHandler;
>
> @@ -2657,26 +2651,6 @@ I830BlockHandler(int i,
>      if (pI830->accel == ACCEL_UXA)
>  	i830_uxa_block_handler (pScreen);
>  #endif
> -    /*
> -     * Check for FIFO underruns at block time (which amounts to just
> -     * periodically).  If this happens, it means our DSPARB or some other
> -     * memory arbitration setting is wrong for the current configuration
> -     * (except for mode setting, where it may occur naturally).
> -     * Check & ack the condition.
> -     */
> -    if (!pI830->use_drm_mode && pScrn->vtSema && !DSPARB_HWCONTROL(pI830))
> { -	if (xf86_config->crtc[0]->enabled &&
> -		(INREG(PIPEASTAT) & FIFO_UNDERRUN)) {
> -	    xf86DrvMsg(pScrn->scrnIndex, X_ERROR, "underrun on pipe A!\n");
> -	    OUTREG(PIPEASTAT, INREG(PIPEASTAT) | FIFO_UNDERRUN);
> -	}
> -	if (xf86_config->num_crtc > 1 &&
> -		xf86_config->crtc[1]->enabled &&
> -		(INREG(PIPEBSTAT) & FIFO_UNDERRUN)) {
> -	    xf86DrvMsg(pScrn->scrnIndex, X_ERROR, "underrun on pipe B!\n");
> -	    OUTREG(PIPEBSTAT, INREG(PIPEBSTAT) | FIFO_UNDERRUN);
> -	}
> -    }
>
>      I830VideoBlockHandler(i, blockData, pTimeout, pReadmask);
>  }

If we do this (patch seems fine) we should check for it in the kernel instead, 
maybe by enabling the corresponding error interrupt.

-- 
Jesse Barnes, Intel Open Source Technology Center