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.
> Signed-off-by: Jesse Barnes <jbar...@virtuousgeek.org>
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index cbb8224..2518ebd 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -829,6 +829,117 @@ static int i915_set_status_page(struct drm_device *dev,
> void *data,
> return 0;
> }
>
> +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;
> +
> + reg = I915_READ(DSPBCNTR);
> + if ((reg & DISPLAY_PLANE_ENABLE) &&
> + ((reg & DISPPLANE_SEL_PIPE_MASK) == pipe_mask))
> + return 1;
> +
> + return -1;
> +}
> +
> +static int i915_gem_page_flip(struct drm_device *dev, void *data,
> + struct drm_file *file_priv)
> +{
> + struct drm_i915_gem_page_flip *flip_data = data;
> + drm_i915_private_t *dev_priv = dev->dev_private;
> + struct drm_gem_object *obj;
> + struct drm_i915_gem_object *obj_priv;
> + unsigned long flags;
> + uint32_t offset;
> + int pitch, pipe, plane, tiled;
> + int ret = 0;
> + RING_LOCALS;
> +
> + if (!(dev->driver->driver_features & DRIVER_GEM))
> + return -ENODEV;
> +
> + /*
> + * Reject unknown flags so future userspace knows what we (don't)
> + * support
> + */
> + if (flip_data->flags & (~I915_PAGE_FLIP_WAIT)) {
> + DRM_ERROR("bad page flip flags\n");
> + return -EINVAL;
> + }
> +
> + if ((pipe = flip_data->pipe) > 1) {
> + DRM_ERROR("bad pipe\n");
> + return -EINVAL;
> + }
> +
> + 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.
> + 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)?
> + 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.
> + /*
> + * 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.
> +
> + 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).
> + ADVANCE_LP_RING();
> +
> + list_add_tail(&obj_priv->vblank_head, &dev_priv->mm.vblank_list[pipe]);
> + drm_vblank_get(dev, pipe);
> +
> + spin_unlock_irqrestore(&dev_priv->vblank_lock, flags);
> +
> + if (flip_data->flags & I915_PAGE_FLIP_WAIT)
> + wait_for_completion(&obj_priv->vblank);
> +
> +out_unref:
> + mutex_lock(&dev->struct_mutex);
> + drm_gem_object_unreference(obj);
> + mutex_unlock(&dev->struct_mutex);
> +
> + return ret;
> +}
> +
> /**
> * i915_probe_agp - get AGP bootup configuration
> * @pdev: PCI device
> @@ -1300,6 +1411,7 @@ struct drm_ioctl_desc i915_ioctls[] = {
> DRM_IOCTL_DEF(DRM_I915_GEM_SET_TILING, i915_gem_set_tiling, 0),
> DRM_IOCTL_DEF(DRM_I915_GEM_GET_TILING, i915_gem_get_tiling, 0),
> DRM_IOCTL_DEF(DRM_I915_GEM_GET_APERTURE, i915_gem_get_aperture_ioctl,
> 0),
> + DRM_IOCTL_DEF(DRM_I915_GEM_PAGE_FLIP, i915_gem_page_flip,
> DRM_AUTH|DRM_MASTER),
> };
>
> int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a8d3256..c77730b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -161,6 +161,9 @@ typedef struct drm_i915_private {
> u32 irq_mask_reg;
> u32 pipestat[2];
>
> + /** Protects vblank list */
> + spinlock_t vblank_lock;
> +
> u32 hotplug_supported_mask;
> struct work_struct hotplug_work;
>
> @@ -341,6 +344,11 @@ typedef struct drm_i915_private {
> */
> struct delayed_work retire_work;
>
> + /**
> + * List of objects waiting on vblank events (one per pipe)
> + */
> + struct list_head vblank_list[2];
> +
> uint32_t next_gem_seqno;
>
> /**
> @@ -392,6 +400,9 @@ struct drm_i915_gem_object {
> /** This object's place on the active/flushing/inactive lists */
> struct list_head list;
>
> + /** Object's place on the vblank list */
> + struct list_head vblank_head;
> +
> /**
> * This is set if the object is on the active or flushing lists
> * (has pending rendering), and is not set if it's on inactive (ready
> @@ -460,6 +471,9 @@ struct drm_i915_gem_object {
>
> /** for phy allocated objects */
> struct drm_i915_gem_phys_object *phys_obj;
> +
> + /** for page flips and other vblank related blocks */
> + struct completion vblank;
> };
>
> /**
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a3df37c..48a556f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2459,6 +2459,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> struct drm_i915_gem_exec_object *exec_list = NULL;
> struct drm_gem_object **object_list = NULL;
> struct drm_gem_object *batch_obj;
> + unsigned long irqflags;
> int ret, i, pinned = 0;
> uint64_t exec_offset;
> uint32_t seqno, flush_domains;
> @@ -2529,6 +2530,17 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> for (pin_tries = 0; ; pin_tries++) {
> ret = 0;
> for (i = 0; i < args->buffer_count; i++) {
> + struct drm_i915_gem_object *obj_priv;
> +
> + 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.)
> object_list[i]->pending_read_domains = 0;
> object_list[i]->pending_write_domain = 0;
> ret = i915_gem_object_pin_and_relocate(object_list[i],
> @@ -2906,6 +2918,9 @@ int i915_gem_init_object(struct drm_gem_object *obj)
> obj_priv->obj = obj;
> obj_priv->fence_reg = I915_FENCE_REG_NONE;
> INIT_LIST_HEAD(&obj_priv->list);
> + INIT_LIST_HEAD(&obj_priv->vblank_head);
> +
> + init_completion(&obj_priv->vblank);
>
> return 0;
> }
> @@ -3327,6 +3342,8 @@ i915_gem_load(struct drm_device *dev)
> INIT_LIST_HEAD(&dev_priv->mm.flushing_list);
> INIT_LIST_HEAD(&dev_priv->mm.inactive_list);
> INIT_LIST_HEAD(&dev_priv->mm.request_list);
> + INIT_LIST_HEAD(&dev_priv->mm.vblank_list[0]);
> + INIT_LIST_HEAD(&dev_priv->mm.vblank_list[1]);
Please add spin_lock_init(&dev_priv->vblank);!
> INIT_DELAYED_WORK(&dev_priv->mm.retire_work,
> i915_gem_retire_work_handler);
> dev_priv->mm.next_gem_seqno = 1;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 019140e..96a7b38 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -196,6 +196,22 @@ static void i915_hotplug_work_func(struct work_struct
> *work)
> drm_sysfs_hotplug_event(dev);
> }
>
> +static void i915_complete_vblank(struct drm_device *dev, int pipe)
> +{
> + drm_i915_private_t *dev_priv = dev->dev_private;
> + struct drm_i915_gem_object *obj_priv, *tmp;
> + unsigned long irqflags;
> +
> + spin_lock_irqsave(&dev_priv->vblank_lock,irqflags);
> + list_for_each_entry_safe(obj_priv, tmp, &dev_priv->mm.vblank_list[pipe],
> + vblank_head) {
> + complete(&obj_priv->vblank);
> + list_del_init(&obj_priv->vblank_head);
> + drm_vblank_put(dev, pipe);
> + }
> + spin_unlock_irqrestore(&dev_priv->vblank_lock,irqflags);
> +}
> +
> irqreturn_t i915_driver_irq_handler(DRM_IRQ_ARGS)
> {
> struct drm_device *dev = (struct drm_device *) arg;
> @@ -284,11 +300,13 @@ irqreturn_t i915_driver_irq_handler(DRM_IRQ_ARGS)
>
> if (pipea_stats & vblank_status) {
> vblank++;
> + i915_complete_vblank(dev, 0);
> drm_handle_vblank(dev, 0);
> }
>
> if (pipeb_stats & vblank_status) {
> vblank++;
> + i915_complete_vblank(dev, 1);
> drm_handle_vblank(dev, 1);
> }
>
> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index 912cd52..0678e47 100644
> --- a/include/drm/i915_drm.h
> +++ b/include/drm/i915_drm.h
> @@ -184,6 +184,7 @@ typedef struct _drm_i915_sarea {
> #define DRM_I915_GEM_GET_TILING 0x22
> #define DRM_I915_GEM_GET_APERTURE 0x23
> #define DRM_I915_GEM_MMAP_GTT 0x24
> +#define DRM_I915_GEM_PAGE_FLIP 0x25
>
> #define DRM_IOCTL_I915_INIT DRM_IOW( DRM_COMMAND_BASE +
> DRM_I915_INIT, drm_i915_init_t)
> #define DRM_IOCTL_I915_FLUSH DRM_IO ( DRM_COMMAND_BASE +
> DRM_I915_FLUSH)
> @@ -219,6 +220,7 @@ typedef struct _drm_i915_sarea {
> #define DRM_IOCTL_I915_GEM_SET_TILING DRM_IOWR (DRM_COMMAND_BASE +
> DRM_I915_GEM_SET_TILING, struct drm_i915_gem_set_tiling)
> #define DRM_IOCTL_I915_GEM_GET_TILING DRM_IOWR (DRM_COMMAND_BASE +
> DRM_I915_GEM_GET_TILING, struct drm_i915_gem_get_tiling)
> #define DRM_IOCTL_I915_GEM_GET_APERTURE DRM_IOR (DRM_COMMAND_BASE +
> DRM_I915_GEM_GET_APERTURE, struct drm_i915_gem_get_aperture)
> +#define DRM_IOCTL_I915_GEM_PAGE_FLIP DRM_IOW (DRM_COMMAND_BASE +
> DRM_I915_GEM_PAGE_FLIP, struct drm_i915_gem_page_flip)
>
> /* Allow drivers to submit batchbuffers directly to hardware, relying
> * on the security mechanisms provided by hardware.
> @@ -654,4 +656,27 @@ struct drm_i915_gem_get_aperture {
> uint64_t aper_available_size;
> };
>
> +#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;
> +};
> +
> #endif /* _I915_DRM_H_ */
>
> ------------------------------------------------------------------------------
> Open Source Business Conference (OSBC), March 24-25, 2009, San Francisco, CA
> -OSBC tackles the biggest issue in open source: Open Sourcing the Enterprise
> -Strategies to boost innovation and cut costs with open source participation
> -Receive a $600 discount off the registration fee with the source code: SFAD
> http://p.sf.net/sfu/XcvMzF8H
> --
> _______________________________________________
> Dri-devel mailing list
> Dri-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/dri-devel
------------------------------------------------------------------------------
Open Source Business Conference (OSBC), March 24-25, 2009, San Francisco, CA
-OSBC tackles the biggest issue in open source: Open Sourcing the Enterprise
-Strategies to boost innovation and cut costs with open source participation
-Receive a $600 discount off the registration fee with the source code: SFAD
http://p.sf.net/sfu/XcvMzF8H
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
