Re: drm: Branch 'vblank-rework'

[Jesse B. <jes...@in...>]

On Wednesday, June 13, 2007 12:24:05 Michel D=C3=A4nzer 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 =3D -ENOMEM;
> > +
> > +	init_waitqueue_head(&dev->vbl_queue);
> > +	spin_lock_init(&dev->vbl_lock);
> > +	atomic_set(&dev->vbl_pending, 0);
> > +	dev->num_crtcs =3D num_crtcs;
> > +
> > +	dev->vbl_sigs =3D drm_alloc(sizeof(struct list_head) * num_crtcs,
> > +				  DRM_MEM_DRIVER);
>
> [...]
>
> > +	ret =3D 0;
> > +	goto out;
>
> Just return 0? :)

Sure, I was just trying to avoid having more than one return, but I=20
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 =3D
> >  		     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 =3D 0; i < 2; i++) {
> > +	for (i =3D 0; i < dev->num_crtcs; i++) {
> >  		drm_vbl_sig_t *vbl_sig, *tmp;
> > -		struct list_head *vbl_sigs =3D i ? &dev->vbl_sigs2 :
> > &dev->vbl_sigs; +		struct list_head *vbl_sigs =3D &dev->vbl_sigs[i];
> >  		unsigned int vbl_seq =3D 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=20
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 =3D (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 =3D atomic_read(&dev->vblank_count[pipe]);
> >
> >  	if (seqtype =3D=3D _DRM_VBLANK_RELATIVE)
> > @@ -670,6 +649,7 @@ int i915_vblank_swap(DRM_IOCTL_ARGS)
> >  			swap.sequence =3D 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. =20
I'll fix that up too.  Thanks again for reviewing so carefully, keep it=20
up! :)

Thanks,
Jesse