Re: [PATCH 5/6] drm/i915: Handle PendingFlip on gen3 robustly

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Tue, 24 Apr 2012 18:31:30 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b378555..47a540a 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2307,7 +2307,7 @@ static void i915_irq_preinstall(struct drm_device * dev)
>  		I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
>  	}
>  
> -	I915_WRITE(HWSTAM, 0xeffe);
> +	I915_WRITE16(HWSTAM, 0xeffe);
>  	for_each_pipe(pipe)
>  		I915_WRITE(PIPESTAT(pipe), 0);
>  	I915_WRITE(IMR, 0xffffffff);

Separate patch.

> @@ -2318,39 +2318,35 @@ static void i915_irq_preinstall(struct drm_device * dev)
>  static int i915_irq_postinstall(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> -	u32 enable_mask = I915_INTERRUPT_ENABLE_FIX | I915_INTERRUPT_ENABLE_VAR;
> -	u32 error_mask;
> +	u32 enable_mask;
>  
>  	dev_priv->vblank_pipe = DRM_I915_VBLANK_PIPE_A | DRM_I915_VBLANK_PIPE_B;
>  
> -	/* Unmask the interrupts that we always want on. */
> -	dev_priv->irq_mask = ~I915_INTERRUPT_ENABLE_FIX;

I'd really like to get rid of these defines at the top of i915_irq.c.
Some are unused and the others just make you check for the right bits
everytime your read the code.

But that can be a separate patch.


> -
>  	dev_priv->pipestat[0] = 0;
>  	dev_priv->pipestat[1] = 0;
>  
> +	I915_WRITE(EMR, ~(I915_ERROR_PAGE_TABLE | I915_ERROR_MEMORY_REFRESH));
> +
> +	/* Unmask the interrupts that we always want on. */
> +	dev_priv->irq_mask =
> +		~(I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
> +		  I915_DISPLAY_PIPE_B_EVENT_INTERRUPT |
> +		  I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
> +		  I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT |
> +		  I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT);

ASLE doesn't exist on gen3?  /me pulls out the docs

Gen3 *does* have ASLE, so don't you need to preserve that bit?

> +
> +	enable_mask =
> +		I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |
> +		I915_DISPLAY_PIPE_B_EVENT_INTERRUPT |
> +		I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT |
> +		I915_USER_INTERRUPT;

Same here.


>  	I915_WRITE(IMR, dev_priv->irq_mask);
>  	I915_WRITE(IER, enable_mask);
>  	POSTING_READ(IER);
> @@ -2371,13 +2367,6 @@ static int i915_irq_postinstall(struct drm_device *dev)
>  			hotplug_en |= SDVOB_HOTPLUG_INT_EN;
>  		if (dev_priv->hotplug_supported_mask & CRT_HOTPLUG_INT_STATUS) {
>  			hotplug_en |= CRT_HOTPLUG_INT_EN;
> -
> -			/* Programming the CRT detection parameters tends
> -			   to generate a spurious hotplug event about three
> -			   seconds later.  So just do it once.
> -			*/
> -			if (IS_G4X(dev))
> -				hotplug_en |= CRT_HOTPLUG_ACTIVATION_PERIOD_64;

This looks like it belongs in a separate "remove gen4 bits from gen3
code" patch.

>  			hotplug_en |= CRT_HOTPLUG_VOLTAGE_COMPARE_50;
>  		}
>  
> @@ -2396,26 +2385,25 @@ static irqreturn_t i915_irq_handler(DRM_IRQ_ARGS)
>  	struct drm_device *dev = (struct drm_device *) arg;
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>  	struct drm_i915_master_private *master_priv;
> -	u32 iir, new_iir;
> -	u32 pipe_stats[I915_MAX_PIPES];
> -	u32 vblank_status;
> -	int vblank = 0;
> +	u32 iir, new_iir, pipe_stats[I915_MAX_PIPES];
>  	unsigned long irqflags;
> -	int irq_received;
> -	int ret = IRQ_NONE, pipe;
> -	bool blc_event = false;
> +	u32 flip_mask =
> +		I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
> +		I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
> +	u32 flip[2] = {
> +		I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT,
> +		I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT
> +	};
> +	int  pipe;
>  
>  	atomic_inc(&dev_priv->irq_received);
>  
>  	iir = I915_READ(IIR);
> +	if ((iir & ~flip_mask) == 0)
> +		return IRQ_NONE;

Note that it's possible for a pipestat bit to get stuck causing an
interrupt even though IIR is clear, causing an interrupt storm and
eventual shutdown of the IRQ.

The safest thing to check here would be the IRQ status bit in PCI
config space.  If that indicates an interrupt is active, then it's
definitely ours, even if IIR or pipestat are busted somehow...  But I
haven't tested how slow config space accesses are for us so it may not
be appropriate for the interrupt handler.

>  
> -	if (INTEL_INFO(dev)->gen >= 4)
> -		vblank_status = PIPE_START_VBLANK_INTERRUPT_STATUS;
> -	else
> -		vblank_status = PIPE_VBLANK_INTERRUPT_STATUS;
> -
> -	for (;;) {
> -		irq_received = iir != 0;
> +	while (iir & ~flip_mask) {
> +		bool blc_event = false;
>  
>  		/* Can't rely on pipestat interrupt bit in iir as it might
>  		 * have been cleared after the pipestat interrupt was received.
> @@ -2438,16 +2426,10 @@ static irqreturn_t i915_irq_handler(DRM_IRQ_ARGS)
>  					DRM_DEBUG_DRIVER("pipe %c underrun\n",
>  							 pipe_name(pipe));
>  				I915_WRITE(reg, pipe_stats[pipe]);
> -				irq_received = 1;
>  			}
>  		}
>  		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
> -		if (!irq_received)
> -			break;
> -
> -		ret = IRQ_HANDLED;
> -
>  		/* Consume port.  Then clear IIR or we'll miss events */
>  		if ((I915_HAS_HOTPLUG(dev)) &&
>  		    (iir & I915_DISPLAY_PORT_INTERRUPT)) {
> @@ -2460,43 +2442,25 @@ static irqreturn_t i915_irq_handler(DRM_IRQ_ARGS)
>  					   &dev_priv->hotplug_work);
>  
>  			I915_WRITE(PORT_HOTPLUG_STAT, hotplug_status);
> -			I915_READ(PORT_HOTPLUG_STAT);
> +			POSTING_READ(PORT_HOTPLUG_STAT);
>  		}
>  
> -		I915_WRITE(IIR, iir);
> +		I915_WRITE(IIR, iir & ~flip_mask);
>  		new_iir = I915_READ(IIR); /* Flush posted writes */
>  
> -		if (dev->primary->master) {
> -			master_priv = dev->primary->master->driver_priv;
> -			if (master_priv->sarea_priv)
> -				master_priv->sarea_priv->last_dispatch =
> -					READ_BREADCRUMB(dev_priv);
> -		}

Separate but worthy patch.  In fact can we get rid of these at all?
Does anyone still pretend that DRI1 works?

> -
>  		if (iir & I915_USER_INTERRUPT)
>  			notify_ring(dev, &dev_priv->ring[RCS]);
> -		if (iir & I915_BSD_USER_INTERRUPT)
> -			notify_ring(dev, &dev_priv->ring[VCS]);

More "remove gen4 from gen3" bits.

> -
> -		if (iir & I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT) {
> -			intel_prepare_page_flip(dev, 0);
> -			if (dev_priv->gen3_flip_pending_is_done)
> -				intel_finish_page_flip_plane(dev, 0);
> -		}
> -
> -		if (iir & I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT) {
> -			intel_prepare_page_flip(dev, 1);
> -			if (dev_priv->gen3_flip_pending_is_done)
> -				intel_finish_page_flip_plane(dev, 1);
> -		}
>  
>  		for_each_pipe(pipe) {
> -			if (pipe_stats[pipe] & vblank_status &&
> +			int plane = pipe;
> +			if (IS_MOBILE(dev))
> +				plane = !plane;

Is this shared with 8xx?  Do we swap every gen3?  Maybe we should just
get rid of the swapping code if FBC is disabled on gen3 by default
anyway...

> +			if (pipe_stats[pipe] & PIPE_VBLANK_INTERRUPT_STATUS &&
>  			    drm_handle_vblank(dev, pipe)) {
> -				vblank++;
> -				if (!dev_priv->gen3_flip_pending_is_done) {
> -					i915_pageflip_stall_check(dev, pipe);
> +				if (iir & flip[plane]) {
> +					intel_prepare_page_flip(dev, plane);
>  					intel_finish_page_flip(dev, pipe);
> +					flip_mask &= ~flip[plane];
>  				}

IIRC the ECOSKPD bit actually did have meaning on some machines.  For
some reporters, tearing was the result of ignoring the difference, so
we'll need to get lots of QA.

>  			}
>  
> @@ -2504,7 +2468,6 @@ static irqreturn_t i915_irq_handler(DRM_IRQ_ARGS)
>  				blc_event = true;
>  		}
>  
> -
>  		if (blc_event || (iir & I915_ASLE_INTERRUPT))
>  			intel_opregion_asle_intr(dev);

Spurious space removal.

>  
> @@ -2526,18 +2489,21 @@ static irqreturn_t i915_irq_handler(DRM_IRQ_ARGS)
>  		iir = new_iir;
>  	}
>  
> -	return ret;
> -}
> +	if (dev->primary->master) {
> +		master_priv = dev->primary->master->driver_priv;
> +		if (master_priv->sarea_priv)
> +			master_priv->sarea_priv->last_dispatch =
> +				READ_BREADCRUMB(dev_priv);
> +	}
>  
> +	return IRQ_HANDLED;
> +}
>  
>  static void i915_irq_uninstall(struct drm_device * dev)
>  {
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>  	int pipe;
>  
> -	if (!dev_priv)
> -		return;
> -
>  	dev_priv->vblank_pipe = 0;
>  
>  	if (I915_HAS_HOTPLUG(dev)) {
> @@ -2545,15 +2511,14 @@ static void i915_irq_uninstall(struct drm_device * dev)
>  		I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
>  	}
>  
> -	I915_WRITE(HWSTAM, 0xffffffff);
> -	for_each_pipe(pipe)
> +	I915_WRITE16(HWSTAM, 0xffff);
> +	for_each_pipe(pipe) {
> +		/* Clear enable bits; then clear status bits */
>  		I915_WRITE(PIPESTAT(pipe), 0);
> +		I915_WRITE(PIPESTAT(pipe), I915_READ(PIPESTAT(pipe)));
> +	}

A good addition mixed in with the 16 bit change.

>  	I915_WRITE(IMR, 0xffffffff);
>  	I915_WRITE(IER, 0x0);
> -
> -	for_each_pipe(pipe)
> -		I915_WRITE(PIPESTAT(pipe),
> -			   I915_READ(PIPESTAT(pipe)) & 0x8000ffff);
>  	I915_WRITE(IIR, I915_READ(IIR));
>  }
>  
> @@ -2740,26 +2705,18 @@ static irqreturn_t i965_irq_handler(DRM_IRQ_ARGS)
>  		if (iir & I915_BSD_USER_INTERRUPT)
>  			notify_ring(dev, &dev_priv->ring[VCS]);
>  
> -		if (iir & I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT) {
> +		if (iir & I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT)
>  			intel_prepare_page_flip(dev, 0);
> -			if (dev_priv->gen3_flip_pending_is_done)
> -				intel_finish_page_flip_plane(dev, 0);
> -		}
>  
> -		if (iir & I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT) {
> +		if (iir & I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT)
>  			intel_prepare_page_flip(dev, 1);
> -			if (dev_priv->gen3_flip_pending_is_done)
> -				intel_finish_page_flip_plane(dev, 1);
> -		}
>  
>  		for_each_pipe(pipe) {
>  			if (pipe_stats[pipe] & vblank_status &&
>  			    drm_handle_vblank(dev, pipe)) {
>  				vblank++;
> -				if (!dev_priv->gen3_flip_pending_is_done) {
> -					i915_pageflip_stall_check(dev, pipe);
> -					intel_finish_page_flip(dev, pipe);
> -				}
> +				i915_pageflip_stall_check(dev, pipe);
> +				intel_finish_page_flip(dev, pipe);
>  			}

These look like "remove gen3 bits from gen4" patch hunks?  If you kept
the field until it was dropped (if we can drop it) you could do it as a
separate patch at the end.

>  
>  			if (pipe_stats[pipe] & PIPE_LEGACY_BLC_EVENT_STATUS)
> @@ -2826,10 +2783,6 @@ void intel_irq_init(struct drm_device *dev)
>  	INIT_WORK(&dev_priv->error_work, i915_error_work_func);
>  	INIT_WORK(&dev_priv->rps_work, gen6_pm_rps_work);
>  
> -	/* IIR "flip pending" bit means done if this bit is set */
> -	if (IS_GEN3(dev) && (I915_READ(ECOSKPD) & ECO_FLIP_DONE))
> -		dev_priv->gen3_flip_pending_is_done = true;
> -
>  	dev->driver->get_vblank_counter = i915_get_vblank_counter;
>  	dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
>  	if (IS_G4X(dev) || IS_GEN5(dev) || IS_GEN6(dev) || IS_IVYBRIDGE(dev) ||
> @@ -2873,6 +2826,9 @@ void intel_irq_init(struct drm_device *dev)
>  			dev->driver->irq_handler = i8xx_irq_handler;
>  			dev->driver->irq_uninstall = i8xx_irq_uninstall;
>  		} else if (INTEL_INFO(dev)->gen == 3) {
> +			/* "flip pending" bit means done if this bit is set */
> +			I915_WRITE(ECOSKPD, _MASKED_BIT_DISABLE(ECO_FLIP_DONE));
> +

Aha.  I hope this works across platforms.  I don't even know why this
bit exists, so I hope it's safe to change on a given chipset & platform.

-- 
Jesse Barnes, Intel Open Source Technology Center