[Intel-gfx] [PATCH] i915: add page flipping ioctl

[Jesse Barnes]

On Sunday, February 15, 2009 4:02 pm Chris Wilson wrote:
> On Thu, 2009-02-12 at 16:52 -0800, Jesse Barnes wrote:
> > This adds a new ioctl for queueing a page flip with GEM objects.  There's
> > a completion per private object, that we use at execbuf time to wait for
> > any pending flips; it's cleared at vblank interrupt time when the flip
> > occurs. The kernel will figure out the pitch of the new frontbuffer
> > automatically, but the caller has to pass in dimensions and pipe
> > information.
>
> On my i915, the flip never occurs and we wait forever on the the vblank.
> So I presume the command we sent the chip is invalid, or we miss the
> irq? (I double-checked with lockdep in case I had missed an obvious
> dead-lock.) Comments on the patch itself inline.

Thanks a lot for looking at this and testing.

Hm, maybe you're not getting vblank interrupts at all?  Do you see the count 
increasing?  Is the IRQ registered?

> > +static int i915_pipe_to_plane(struct drm_device *dev, int pipe)
> > +{
> > +	drm_i915_private_t *dev_priv = dev->dev_private;
> > +	u32 reg, pipe_mask;
> > +
> > +	if (pipe == 0)
> > +		pipe_mask = DISPPLANE_SEL_PIPE_A;
> > +	else
> > +		pipe_mask = DISPPLANE_SEL_PIPE_B;
> > +
> > +	reg = I915_READ(DSPACNTR);
> > +	if ((reg & DISPLAY_PLANE_ENABLE) &&
> > +	    ((reg & DISPPLANE_SEL_PIPE_MASK) == pipe_mask))
> > +			return 0;
>
> Would this be easier to read as:
>   pipe_mask |= DISPLAY_PLANE_ENABLE;
>
>   reg = I915_READ(DSPACNTR);
>   if ((reg & (DISPLAY_PLANE_ENABLE | DISPPLANE_SEL_PIPE_MASK)) ==
> pipe_mask) return 0;
>
>   reg = I915_READ(DSPBCNTR);
>   if ((reg & (DISPLAY_PLANE_ENABLE | DISPPLANE_SEL_PIPE_MASK)) ==
> pipe_mask) return 1;

Yeah, looks a bit clearer that way.

> > +	plane = i915_pipe_to_plane(dev, pipe);
> > +	if (plane < 0) {
> > +		DRM_ERROR("bad pipe (no planes enabled?)\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	obj = drm_gem_object_lookup(dev, file_priv, flip_data->handle);
> > +	if (obj == NULL)
> > +		return -EBADF;
> > +
> > +	obj_priv = obj->driver_private;
>
> Need to take the dev->struct_mutex here whilst reading the tiling
> parameters and pinning.

Oops yeah will fix.

> > +	if (obj_priv->tiling_mode != I915_TILING_NONE &&
> > +	    obj_priv->tiling_mode != I915_TILING_X) {
> > +		DRM_ERROR("can only flip non-tiled or X tiled pages\n");
> > +		ret = -EINVAL;
> > +		goto out_unref;
> > +	}
> > +
> > +	ret = i915_gem_object_pin(obj, 0);
>
> Since we do not appear to explicitly track and unpin the old scan-out
> buffer, presumably we are just reliant on user space destroying the old
> buffers in a timely manner in order to recover their gtt space (and the
> precious fence register)?

Yeah, I'm open to suggestions there...  Current (i.e. non-KMS) userspace pins 
the buffers itself, but it *should* be ok to unpin the old buffer at 
completion time.  I'll have to give that a try.

As for tiling, we really need to change Mesa to use 3D blits (with 
corresponding tile parameters) so that we don't have to use the fence regs as 
much.  But even so, unpinning and removing the fence reg from the old buffer 
would be a good thing.

> > +	if (ret) {
> > +		DRM_ERROR("failed to pin object for flip\n");
> > +		ret = -EBUSY;
> > +		goto out_unref;
> > +	}
> > +
> > +	offset = obj_priv->gtt_offset;
> > +	pitch = obj_priv->stride;
> > +	tiled = !!(obj_priv->tiling_mode == I915_TILING_X);
>
> Having pinned the buffer, we can now drop the mutex.

Ok.

> > +	/*
> > +	 * Queue the flip with the lock held in case the flip happens
> > +	 * immediately.
> > +	 */
> > +	spin_lock_irqsave(&dev_priv->vblank_lock, flags);
>
> vblank_lock is never initialised.

Heh, yeah I realized that just after I sent it out.

>
> > +
> > +	BEGIN_LP_RING(4);
> > +	OUT_RING(CMD_OP_DISPLAYBUFFER_INFO | (plane << 20));
> > +	OUT_RING((pitch / 8) << 3); /* flip queue and/or pitch */
> > +	OUT_RING(offset | tiled);
> > +	OUT_RING(((flip_data->x - 1) << 16) | (flip_data->y - 1));
>
> x/y look reversed compared to other commands (but I'm just speculating
> without the documentation).

According to the public docs, X is 27:16 and Y is 11:0, so I *think* it's 
right (though it appears previous chips don't have this field).

> > +			obj_priv = object_list[i]->driver_private;
> > +			spin_lock_irqsave(&dev_priv->vblank_lock, irqflags);
> > +			if (!list_empty(&obj_priv->vblank_head)) {
> > +				spin_unlock_irqrestore(&dev_priv->vblank_lock,
> > +						       irqflags);
> > +				wait_for_completion(&obj_priv->vblank);
> > +			} else
> > +				spin_unlock_irqrestore(&dev_priv->vblank_lock,
> > +						       irqflags);
>
> This is quite untidy. Move it to an inline? (And reduce it to only need
> a single unlock.)

Yeah was thinking the same thing; should just be if (flip_pending(&obj_priv)) 
or something instead, followed by the wait.

> > +#define I915_PAGE_FLIP_WAIT (1<<0) /* block on page flip completion */
> > +
> > +struct drm_i915_gem_page_flip {
> > +	/** Handle of new front buffer */
> > +	uint32_t handle;
> > +
> > +	/**
> > +	 * page flip flags (wait on flip only for now)
> > +	 */
> > +	uint32_t flags;
> > +
> > +	/**
> > +	 * pipe to flip
> > +	 */
> > +	uint32_t pipe;
> > +
> > +	/**
> > +	 * screen dimensions for flip
> > +	 */
> > +	uint32_t x;
> > +	uint32_t y;
> > +};
> > +

The other piece, of course, is to come up with a KMS API; the current mode set 
interface *should* be sufficient for flipping, but afaik it's only been 
tested with panning flips, so may need some work.  An alternative would be to 
make this API also work w/KMS, which might mean re-defining the "pipe" field 
to a CRTC id if KMS is active...  Any thoughts?

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center