On Tue, 2007-06-12 at 13:37 -0700, Jesse Barnes wrote:
>
> diff --git a/linux-core/drm_irq.c b/linux-core/drm_irq.c
> index f229f77..8125b75 100644
> --- a/linux-core/drm_irq.c
> +++ b/linux-core/drm_irq.c
> @@ -77,6 +77,70 @@ int drm_irq_by_busid(struct inode *inode
> return 0;
> }
>
> +int drm_vblank_init(drm_device_t *dev, int num_crtcs)
> +{
> + int i, ret = -ENOMEM;
> +
> + init_waitqueue_head(&dev->vbl_queue);
> + spin_lock_init(&dev->vbl_lock);
> + atomic_set(&dev->vbl_pending, 0);
> + dev->num_crtcs = num_crtcs;
> +
> + dev->vbl_sigs = drm_alloc(sizeof(struct list_head) * num_crtcs,
> + DRM_MEM_DRIVER);
[...]
> + ret = 0;
> + goto out;
Just return 0? :)
> +err:
> + kfree(dev->vbl_sigs);
Mismatch between drm_alloc() and kfree(). If the code stays in a Linux
specific file, you can use kmalloc.
> @@ -359,6 +450,8 @@ int drm_wait_vblank(DRM_IOCTL_ARGS)
>
> spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
>
> + atomic_inc(&dev->vbl_pending);
> +
> if (!
> (vbl_sig =
> drm_alloc(sizeof(drm_vbl_sig_t), DRM_MEM_DRIVER))) {
This increases the count before we know we really schedule a new signal.
(Was broken before the rework, just noticed it now)
> @@ -414,9 +507,9 @@ void drm_vbl_send_signals(drm_device_t *
>
> spin_lock_irqsave(&dev->vbl_lock, flags);
>
> - for (i = 0; i < 2; i++) {
> + for (i = 0; i < dev->num_crtcs; i++) {
> drm_vbl_sig_t *vbl_sig, *tmp;
> - struct list_head *vbl_sigs = i ? &dev->vbl_sigs2 :
> &dev->vbl_sigs;
> + struct list_head *vbl_sigs = &dev->vbl_sigs[i];
> unsigned int vbl_seq = atomic_read(&dev->vblank_count[i]);
>
> list_for_each_entry_safe(vbl_sig, tmp, vbl_sigs, head) {
As has been discussed in other posts, it would probably be nice if
everything including this and the blocking waitqueues was per CRTC. I
think there could be a single drm_vblank_handler(drm_device_t *dev, int
crtc) function to be called from the driver's interrupt handler which
takes care of waking up the CRTC waitqueue and sending its pending
signals.
> +typedef struct drm_modeset_ctl {
> + drm_modeset_ctl_cmd_t cmd;
> + unsigned long arg;
> +} drm_modeset_ctl_t;
unsigned long is bad for 32 bit userland on a 64 bit kernel.
> @@ -953,6 +968,8 @@ typedef union drm_mm_init_arg{
>
> #define DRM_IOCTL_UPDATE_DRAW DRM_IOW(0x3f, drm_update_draw_t)
>
> +#define DRM_IOCTL_MODESET_CTL DRM_IOW(0x40, drm_modeset_ctl_t)
0x40 is the first driver specific ioctl. There's a second driver
independent range starting at 0xa0 (DRM_COMMAND_END).
> + if (temp & VSYNC_PIPEA_FLAG)
> + atomic_add(i915_get_vblank_counter(dev, 0),
> + &dev->vblank_count[0]);
> + if (temp & VSYNC_PIPEB_FLAG)
> + atomic_add(i915_get_vblank_counter(dev, 1),
> + &dev->vblank_count[1]);
I think atomic_add is wrong here.
> @@ -461,6 +481,9 @@ int i915_irq_wait(DRM_IOCTL_ARGS)
> void i915_enable_vblank(drm_device_t *dev, int crtc)
> {
> drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> +
> + if (crtc > dev_priv->vblank_pipe)
> + return;
Should be something like !(dev_priv->vblank_pipe & (1 << crtc)).
> @@ -660,6 +638,7 @@ int i915_vblank_swap(DRM_IOCTL_ARGS)
>
> spin_unlock_irqrestore(&dev->drw_lock, irqflags);
>
> + drm_vblank_get(dev, pipe);
> curseq = atomic_read(&dev->vblank_count[pipe]);
>
> if (seqtype == _DRM_VBLANK_RELATIVE)
> @@ -670,6 +649,7 @@ int i915_vblank_swap(DRM_IOCTL_ARGS)
> swap.sequence = curseq + 1;
> } else {
> DRM_DEBUG("Missed target sequence\n");
> + drm_vblank_put(dev, pipe);
> return DRM_ERR(EINVAL);
> }
> }
I think updating the counters should be split off drm_vblank_get(), so
it can only be called once it's known the interrupt needs to be enabled,
and drm_vblank_put() doesn't need to be added to every error path. (As a
bonus, the interrupt never gets needlessly enabled and immediately
disabled again in case of an error)
--
Earthling Michel Dänzer | http://tungstengraphics.com
Libre software enthusiast | Debian, X and DRI developer
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
