On pe, 2015-02-13 at 21:03 +0100, Daniel Vetter wrote:
> The pipe might already have been shut down, and then it's not a good
> idea to call hw accessor functions. Instead use the same logic as
> drm_vblank_off which has all the necessary checks to avoid troubles or
> inconsistency.
>
> Noticed by Imre while reviewing my patches to remove some sanity
> checks from ->get_vblank_counter.
>
> v2: Try harder. disable_and_save can still access the vblank stuff
> when vblank->enabled isn't set. It has to, since vlbank irq could be
> disable but the pipe is still on when being called from
> drm_vblank_off. But we still want to use that code for more code
> sharing. So add a check for vblank->enabled on top - if that's not set
> we shouldn't have anyone waiting for the vblank. If we have that's a
> pretty serious bug.
>
> The other issue that Imre spotted is drm_vblank_cleanup. That code
> again calls disable_and_save and so suffers from the same issues. But
> really drm_irq_uninstall should have cleaned that all up, so replace
> the code with WARN_ON. Note that we can't delete the timer cleanup
> since drivers aren't required to use drm_irq_install/uninstall, but
> can do their own irq handling.
>
> v3: Make it clear that all that gunk in drm_irq_uninstall is really
> just bandaids for UMS races between the irq/vblank code. In UMS
> userspace is in control of enabling/disabling interrupts in general
> and vblanks specifically.
>
> Cc: Imre Deak <imre.deak@xxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
> drivers/gpu/drm/drm_irq.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 1e5fb1b994d7..885fb756fed5 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -269,7 +269,6 @@ static void vblank_disable_fn(unsigned long arg)
> void drm_vblank_cleanup(struct drm_device *dev)
> {
> int crtc;
> - unsigned long irqflags;
>
> /* Bail if the driver didn't call drm_vblank_init() */
> if (dev->num_crtcs == 0)
> @@ -278,11 +277,9 @@ void drm_vblank_cleanup(struct drm_device *dev)
> for (crtc = 0; crtc < dev->num_crtcs; crtc++) {
> struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
>
> - del_timer_sync(&vblank->disable_timer);
> + WARN_ON(vblank->enabled);
>
> - spin_lock_irqsave(&dev->vbl_lock, irqflags);
> - vblank_disable_and_save(dev, crtc);
> - spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> + del_timer_sync(&vblank->disable_timer);
> }
Looking also at non-i915 drivers, drm_vblank_cleanup is called always
before drm_irq_uninstall. Due to 'dev->num_crtcs = 0' in
drm_vblank_cleanup the loop in drm_irq_uninstall will be bypassed and
vblank_disable_and_save won't be called. This is mainly a concern for
UMS as you commented below, but if we care about that I'd prefer doing
the same thing above as you did in drm_irq_uninstall. Don't do anything
if vblanks are disabled otherwise warn only for KMS and call
vblank_disable_and_save and wake_up the vblank queue.
With that this series looks ok to me.
>
> kfree(dev->vblank);
> @@ -468,17 +465,23 @@ int drm_irq_uninstall(struct drm_device *dev)
> dev->irq_enabled = false;
>
> /*
> - * Wake up any waiters so they don't hang.
> + * Wake up any waiters so they don't hang. This is just to paper over
> + * isssues for UMS drivers which aren't in full control of their
> + * vblank/irq handling. KMS drivers must ensure that vblanks are all
> + * disabled when uninstalling the irq handler.
> */
> if (dev->num_crtcs) {
> spin_lock_irqsave(&dev->vbl_lock, irqflags);
> for (i = 0; i < dev->num_crtcs; i++) {
> struct drm_vblank_crtc *vblank = &dev->vblank[i];
>
> + if (!vblank->enabled)
> + continue;
> +
> + WARN_ON(drm_core_check_feature(dev, DRIVER_MODESET));
> +
> + vblank_disable_and_save(dev, i);
> wake_up(&vblank->queue);
> - vblank->enabled = false;
> - vblank->last =
> - dev->driver->get_vblank_counter(dev, i);
> }
> spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> }
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
