[Intel-gfx] [PATCH] [drm/i915] Handle swap buffers in work proc instead of softirq

[Eric Anholt]

On Fri, 2008-10-17 at 00:44 -0700, Keith Packard wrote:
> Swap buffers must hold both the DRM lock and the struct_mutex to access
> both drawable clip lists and the hardware ring. struct_mutex may only be
> acquired in a non-interrupt context. This means that vblank will be entirely
> executed outside of IRQ context, which is not ideal, but should avoid
> lengthy delays while the ring is full along with making it actually work
> reliably.

OK, this looks a lot nicer.  I also like moving from planes to pipes in
vblank handling code, since it's pipes and not planes that produce
vblank events.  But I fear that there's some pipe/plane confusion going
on here.

> @@ -165,7 +170,7 @@ static void i915_vblank_do_swaps(struct drm_device *dev)
>  	list_for_each_safe(list, tmp, &dev_priv->vbl_swaps.head) {
>  		drm_i915_vbl_swap_t *vbl_swap =
>  			list_entry(list, drm_i915_vbl_swap_t, head);
> -		int pipe = i915_get_pipe(dev, vbl_swap->plane);
> +		int pipe = vbl_swap->pipe;
>  
>  		if ((counter[pipe] - vbl_swap->sequence) > (1<<23))
>  			continue;

OK, so we're getting counter[pipe] = drm_vblank_count(dev, pipe)

The prototype of that one is:

u32 drm_vblank_count(struct drm_device *dev, int crtc)

so you should be in the clear there since a crtc is a pipe not a plane.

> -void
> -i915_gem_vblank_work_handler(struct work_struct *work)
> +i915_vblank_work_handler(struct work_struct *work)
>  {
>  	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
> -						    mm.vblank_work);
> +						    vblank_work);
>  	struct drm_device *dev = dev_priv->dev;
> +	unsigned long irqflags;
>  
> -	if (dev->lock.hw_lock != NULL) {
> -		drm_locked_tasklet(dev, i915_vblank_tasklet);
> +	if (dev->lock.hw_lock == NULL) {
> +		i915_vblank_tasklet(dev);
>  		return;
> -	} else {
> -		mutex_lock(&dev->struct_mutex);
> -		i915_vblank_do_swaps(dev);
> -		mutex_unlock(&dev->struct_mutex);
>  	}
> +	
> +	spin_lock_irqsave(&dev->tasklet_lock, irqflags);
> +	dev->locked_tasklet_func = i915_vblank_tasklet;
> +	spin_unlock_irqrestore(&dev->tasklet_lock, irqflags);
> +
> +	/* Try to get the lock now, if this fails, the lock
> +	 * holder will execute the tasklet during unlock
> +	 */
> +	if (!drm_lock_take(&dev->lock, DRM_KERNEL_CONTEXT))
> +		return;
> +	
> +	dev->lock.lock_time = jiffies;
> +	atomic_inc(&dev->counts[_DRM_STAT_LOCKS]);
> +
> +	spin_lock_irqsave(&dev->tasklet_lock, irqflags);
> +	dev->locked_tasklet_func = NULL;
> +	spin_unlock_irqrestore(&dev->tasklet_lock, irqflags);
> +
> +	i915_vblank_tasklet(dev);
> +	drm_lock_free(&dev->lock, DRM_KERNEL_CONTEXT);
>  }

I had to stare at this one for a bit -- there's a race to get
vblank_tasklet executed twice, but I think that's harmless because the
tasklet should get 0 hits in that case and not do anything.

> -	if (dev_priv->swaps_pending >= 100) {
> +	if (dev_priv->swaps_pending >= 10) {
>  		DRM_DEBUG("Too many swaps queued\n");
> +		DRM_DEBUG(" pipe 0: %d pipe 1: %d\n",
> +			  drm_vblank_count(dev, i915_get_plane(dev, 0)),
> +			  drm_vblank_count(dev, i915_get_plane(dev, 1)));

Here you've got count[pipe] = drm_vblank_count(dev, plane).

That seems wrong.

Other than that, it looks good to me.

-- 
Eric Anholt
eric at anholt.net                         eric.anholt at intel.com


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20081017/bdb03f47/attachment.sig>