On Monday, June 11, 2007 11:59:23 Michel Dänzer wrote:
> On Mon, 2007-06-11 at 15:20 -0700, Jesse Barnes wrote:
> > On Monday, June 11, 2007 11:36:10 Keith Packard wrote:
> > > ick. just read the registers and return the value here. We should
> > > place wrap-detection in the core code by reporting the range of
> > > the register values; with the offset suggested above, that would
> > > result in a single addition to convert from raw to cooked frame
> > > counts.
> >
> > Ok, here's an updated version:
> > - move wraparound logic to the core
> > - add pre/post modeset ioctls (per-driver right now, making them
> > core would mean lots more DDX changes I think),
>
> Shouldn't really matter, DDX drivers can call driver independent
> ioctls.
Yeah, that's true. It could also be called from the server's new
modesetting code, so that converted drivers would automatically
benefit. I just wanted to avoid having to update every driver, but I
think we can avoid that in either case.
> > @@ -265,13 +301,13 @@ int drm_wait_vblank(DRM_IOCTL_ARGS)
> > }
> >
> > flags = vblwait.request.type & _DRM_VBLANK_FLAGS_MASK;
> > + crtc = flags & _DRM_VBLANK_SECONDARY ? 1 : 0;
> >
> > if (!drm_core_check_feature(dev, (flags & _DRM_VBLANK_SECONDARY)
> > ? DRIVER_IRQ_VBL2 : DRIVER_IRQ_VBL))
> > return -EINVAL;
> >
> > - seq = atomic_read((flags & _DRM_VBLANK_SECONDARY) ?
> > &dev->vbl_received2 - : &dev->vbl_received);
> > + seq = atomic_read(&dev->vblank_count[crtc]);
> >
> > switch (vblwait.request.type & _DRM_VBLANK_TYPES_MASK) {
> > case _DRM_VBLANK_RELATIVE:
>
> This value is used as the basis for relative waits, so you need to
> make sure it's up to date.
Yeah, Keith mentioned this too, I've fixed it in my tree.
> > @@ -311,15 +347,15 @@ int drm_wait_vblank(DRM_IOCTL_ARGS)
> > }
> > }
> >
> > - if (dev->vbl_pending >= 100) {
> > + if (atomic_read(&dev->vbl_pending[crtc]) >= 100) {
> > spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> > return -EBUSY;
> > }
>
> Previously, dev->vbl_pending was only used to make sure userspace
> can't exhaust memory by scheduling an infinite number of signals. I
> don't think this is necessary for blocking calls (as every process
> can only do one such call at a time), is it?
I don't think so... we should be able to catch that case through a
regular system wide process count limit (though we'll probably hit a
memory or other resource problem first).
>
> > @@ -313,14 +313,14 @@ irqreturn_t
> > i915_driver_irq_handler(DRM_IRQ_ARGS)
> > (DRM_I915_VBLANK_PIPE_A | DRM_I915_VBLANK_PIPE_B))
> > == (DRM_I915_VBLANK_PIPE_A | DRM_I915_VBLANK_PIPE_B)) {
> > if (temp & VSYNC_PIPEA_FLAG)
> > - atomic_inc(&dev->vbl_received);
> > + atomic_inc(&dev->vblank_count[0]);
> > if (temp & VSYNC_PIPEB_FLAG)
> > - atomic_inc(&dev->vbl_received2);
> > + atomic_inc(&dev->vblank_count[1]);
> > } else if (((temp & VSYNC_PIPEA_FLAG) &&
> > (vblank_pipe & DRM_I915_VBLANK_PIPE_A)) ||
> > ((temp & VSYNC_PIPEB_FLAG) &&
> > (vblank_pipe & DRM_I915_VBLANK_PIPE_B)))
> > - atomic_inc(&dev->vbl_received);
> > + atomic_inc(&dev->vblank_count[0]);
> >
> > DRM_WAKEUP(&dev->vbl_queue);
> > drm_vbl_send_signals(dev);
>
> Maybe this should use i915_get_vblank_counter() instead of just
> incrementing, in case e.g. an interrupt is missed.
Yeah, that's a good idea. And I think it could simplify this logic a
bit?
> > @@ -489,15 +458,116 @@ int i915_irq_wait(DRM_IOCTL_ARGS)
> > return i915_wait_irq(dev, irqwait.irq_seq);
> > }
> >
> > -static void i915_enable_interrupt (drm_device_t *dev)
> > +void i915_enable_vblank(drm_device_t *dev, int crtc)
> > {
> > drm_i915_private_t *dev_priv = (drm_i915_private_t *)
> > dev->dev_private;
> >
> > - dev_priv->irq_enable_reg = USER_INT_FLAG;
> > - if (dev_priv->vblank_pipe & DRM_I915_VBLANK_PIPE_A)
> > + switch (crtc) {
> > + case 0:
> > dev_priv->irq_enable_reg |= VSYNC_PIPEA_FLAG;
> > - if (dev_priv->vblank_pipe & DRM_I915_VBLANK_PIPE_B)
> > + break;
> > + case 1:
> > dev_priv->irq_enable_reg |= VSYNC_PIPEB_FLAG;
> > + break;
> > + default:
> > + DRM_ERROR("tried to enable vblank on non-existent crtc %d\n",
> > + crtc);
> > + break;
> > + }
> > +
> > + I915_WRITE16(I915REG_INT_ENABLE_R, dev_priv->irq_enable_reg);
> > +}
>
> Does this need to check that the X server enabled the vblank
> interrupt for the pipe? What would happen if this ended up enabling
> it for an inactive pipe?
Hm, we might get nonsensical values or a non-incrementing vblank count,
but otoh userspace didn't bother to enable vblank events for the pipe
it just requested one for, so maybe undefined behavior is ok?
If not, it would be easy to check the dev_priv->vblank_pipe value for
sanity here.
>
> > @@ -607,7 +677,7 @@ int i915_vblank_swap(DRM_IOCTL_ARGS)
> >
> > spin_unlock_irqrestore(&dev->drw_lock, irqflags);
> >
> > - curseq = atomic_read(pipe ? &dev->vbl_received2 :
> > &dev->vbl_received); + curseq =
> > atomic_read(&dev->vblank_count[pipe]);
> >
> > if (seqtype == _DRM_VBLANK_RELATIVE)
> > swap.sequence += curseq;
>
> Again, need to make sure this is up to date. This function probably
> needs more changes for the new scheme, I'm actually not quite sure
> yet how it fits in.
Yeah, I didn't look at it carefully. I think it'll also need at
drm_vblank_get with a put in the tasklet, like the signal code.
>
> > @@ -721,6 +792,27 @@ void i915_driver_irq_postinstall(drm_device_t
> > * dev)
> >
> > dev_priv->user_irq_lock = SPIN_LOCK_UNLOCKED;
> > dev_priv->user_irq_refcount = 0;
> > + dev_priv->irq_enable_reg = 0;
> > + dev->vblank_count = kmalloc(sizeof(atomic_t) * 2, GFP_KERNEL);
> > + if (!dev->vblank_count) {
> > + DRM_ERROR("out of memory\n");
> > + return;
> > + }
> > + dev->vbl_pending = kmalloc(sizeof(atomic_t) * 2, GFP_KERNEL);
> > + if (!dev->vbl_pending) {
> > + DRM_ERROR("out of memory\n");
> > + kfree(dev->vblank_count);
> > + return;
> > + }
> > +
> > + /* Zero per-crtc vblank stuff */
> > + for (i = 0; i < 2; i++) {
> > + atomic_set(&dev->vbl_pending[i], 0);
> > + atomic_set(&dev->vblank_count[i], 0);
> > + dev_priv->vblank_offset[i] = 0;
> > + }
> > +
> > + dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count
> > */
>
> This code is shared between OSs, so please use drm_alloc instead of
> kmalloc. Then again, it might be better to do all this in a core
> function instead of duplicating it in each driver.
Yeah, that's a good idea, drm_vblank_init or somesuch that takes a crtc
count.
Thanks a lot for the review. I'll update the code with your
suggestions.
Jesse
-------------------------------------------------------------------------
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
