On Tue, 2007-06-12 at 09:40 -0700, Jesse Barnes wrote:
> On Monday, June 11, 2007 11:59:23 Michel Dänzer wrote:
> > On Mon, 2007-06-11 at 15:20 -0700, Jesse Barnes wrote:
> > >
> > > 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.
Well, driver specific ioctls actually require driver changes both in the
DRM and the DDX, whereas core ioctls shouldn't require any DRM driver
changes and could be handled outside of drivers in the DDX down the road
as well. So the latter seem preferable.
> > > @@ -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).
That's what I was thinking, so I'd say don't bother with that and keep
tracking pending signals separately for this purpose. Otherwise an app
could prevent others from using the blocking interface by scheduling as
many signals as it can.
> > > @@ -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?
Hmm yeah, I guess you could just update both counters unconditionally.
> > > @@ -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?
Yes, I'm not worried about the software side of it, as this should only
occur when there's a bug somewhere. I'm rather wondering whether
enabling the vblank interrupt for an inactive pipe might cause trouble
on the hardware side, but maybe that's not an issue. Keith?
> > > @@ -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.
Right, it's essentially the same thing.
--
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
