On Wednesday, June 13, 2007 12:24:05 Michel Dänzer wrote:
> 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? :)
Sure, I was just trying to avoid having more than one return, but I
don't care too much.
> > +err:
> > + kfree(dev->vbl_sigs);
>
> Mismatch between drm_alloc() and kfree(). If the code stays in a
> Linux specific file, you can use kmalloc.
Oops, yeah that should be fixed.
> > @@ -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)
Ok I'll fix that.
> > @@ -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.
Yeah, it probably won't help much from a performance perspective but
would make the code cleaner.
>
> > +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.
Yeah, it should probably be u64, or a real per-subioctl command union.
>
> > @@ -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).
Oops, ok I'll fix that.
> > + 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.
Hm yeah it should be just atomic_set(), duh.
> > @@ -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)).
Yeah, that's a better check.
> > @@ -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)
Yeah, that's a good idea, much less error prone than what I have now.
I'll fix that up too. Thanks again for reviewing so carefully, keep it
up! :)
Thanks,
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/dri-devel
