[Intel-gfx] [PATCH] drm/i915: don't queue flips during a flip pending event

[Eric Anholt]

On Mon, 5 Apr 2010 14:08:31 -0700, Jesse Barnes <jbarnes at virtuousgeek.org> wrote:
> Hardware will set the flip pending ISR bit as soon as it receives the
> flip instruction, and (supposedly) clear it once the flip completes
> (e.g. at the next vblank).  If we try to send down a flip instruction
> while the ISR bit is set, the hardware can become very confused, and we
> may never receive the corresponding flip pending interrupt, effectively
> hanging the chip.
> 
> Fixes fdo bug #27040.
> 
> Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6f5e7f5..24853e1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4235,6 +4235,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	unsigned long flags;
>  	int pipesrc_reg = (intel_crtc->pipe == 0) ? PIPEASRC : PIPEBSRC;
>  	int ret, pipesrc;
> +	u32 flip_mask;
>  	RING_LOCALS;
>  
>  	work = kzalloc(sizeof *work, GFP_KERNEL);
> @@ -4285,6 +4286,15 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	atomic_inc(&obj_priv->pending_flip);
>  	work->pending_flip_obj = obj;
>  
> +	if (intel_crtc->plane)
> +		flip_mask = I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
> +	else
> +		flip_mask = I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT;
> +
> +	/* Wait for any previous flip to finish */
> +	while (I915_READ(ISR) & flip_mask)
> +		;
> +

I'm confused about how this patch works.  It looks like there's a race
where the ISR bit could get set at this point after the loop checking
it, and you would lose.  The whole thing seems very dubious -- why does
the state of the ISR at the time you queue a flip matter?  I could
imagine there being an issue with the state of the ISR at the point that
a flip packet is processed, but this doesn't handle that (seems like
only throttling flips to avoid outstanding flips would)

>  	BEGIN_LP_RING(4);
>  	if (IS_I965G(dev)) {
>  		OUT_RING(MI_DISPLAY_FLIP |
> -- 
> 1.6.6.1
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20100429/46052d9b/attachment.sig>