Re: [PATCH 07/10] drm/radeon: apply Murphy's law to the kms irq code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, May 24, 2012 at 11:35:15AM -0400, Alex Deucher wrote:
> On Thu, May 24, 2012 at 3:49 AM, Christian König
> <deathsimple@xxxxxxxxxxx> wrote:
> > From: Christian Koenig <christian.koenig@xxxxxxx>
> >
> > 1. It is really dangerous to have more than one
> >   spinlock protecting the same information.
> >
> > 2. radeon_irq_set sometimes wasn't called with lock
> >   protection, so it can happen that more than one
> >   CPU would tamper with the irq regs at the same
> >   time.
> >
> > 3. The pm.gui_idle variable was assuming that the 3D
> >   engine wasn't becoming idle between testing the
> >   register and setting the variable. So just remove
> >   it and test the register directly.
> >
> > Signed-off-by: Christian Koenig <christian.koenig@xxxxxxx>
> > ---
> >  drivers/gpu/drm/radeon/evergreen.c      |    1 -
> >  drivers/gpu/drm/radeon/r100.c           |    1 -
> >  drivers/gpu/drm/radeon/r600.c           |    1 -
> >  drivers/gpu/drm/radeon/r600_hdmi.c      |    6 +--
> >  drivers/gpu/drm/radeon/radeon.h         |   33 +++++++-------
> >  drivers/gpu/drm/radeon/radeon_irq_kms.c |   72 +++++++++++++++++++++++++------
> >  drivers/gpu/drm/radeon/radeon_kms.c     |   12 ++++--
> >  drivers/gpu/drm/radeon/radeon_pm.c      |   12 +-----
> >  drivers/gpu/drm/radeon/rs600.c          |    1 -
> >  drivers/gpu/drm/radeon/si.c             |    1 -
> >  10 files changed, 90 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
> > index bfcb39e..9e9b3bb 100644
> > --- a/drivers/gpu/drm/radeon/evergreen.c
> > +++ b/drivers/gpu/drm/radeon/evergreen.c
> > @@ -3254,7 +3254,6 @@ restart_ih:
> >                        break;
> >                case 233: /* GUI IDLE */
> >                        DRM_DEBUG("IH: GUI idle\n");
> > -                       rdev->pm.gui_idle = true;
> >                        wake_up(&rdev->irq.idle_queue);
> >                        break;
> >                default:
> > diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c
> > index 415b7d8..2587426 100644
> > --- a/drivers/gpu/drm/radeon/r100.c
> > +++ b/drivers/gpu/drm/radeon/r100.c
> > @@ -782,7 +782,6 @@ int r100_irq_process(struct radeon_device *rdev)
> >                /* gui idle interrupt */
> >                if (status & RADEON_GUI_IDLE_STAT) {
> >                        rdev->irq.gui_idle_acked = true;
> > -                       rdev->pm.gui_idle = true;
> >                        wake_up(&rdev->irq.idle_queue);
> >                }
> >                /* Vertical blank interrupts */
> > diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
> > index eadbb06..90c6639 100644
> > --- a/drivers/gpu/drm/radeon/r600.c
> > +++ b/drivers/gpu/drm/radeon/r600.c
> > @@ -3542,7 +3542,6 @@ restart_ih:
> >                        break;
> >                case 233: /* GUI IDLE */
> >                        DRM_DEBUG("IH: GUI idle\n");
> > -                       rdev->pm.gui_idle = true;
> >                        wake_up(&rdev->irq.idle_queue);
> >                        break;
> >                default:
> > diff --git a/drivers/gpu/drm/radeon/r600_hdmi.c b/drivers/gpu/drm/radeon/r600_hdmi.c
> > index 226379e..b76c0f2 100644
> > --- a/drivers/gpu/drm/radeon/r600_hdmi.c
> > +++ b/drivers/gpu/drm/radeon/r600_hdmi.c
> > @@ -523,8 +523,7 @@ void r600_hdmi_enable(struct drm_encoder *encoder)
> >
> >        if (rdev->irq.installed) {
> >                /* if irq is available use it */
> > -               rdev->irq.afmt[dig->afmt->id] = true;
> > -               radeon_irq_set(rdev);
> > +               radeon_irq_kms_enable_afmt(rdev, dig->afmt->id);
> >        }
> >
> >        dig->afmt->enabled = true;
> > @@ -560,8 +559,7 @@ void r600_hdmi_disable(struct drm_encoder *encoder)
> >                  offset, radeon_encoder->encoder_id);
> >
> >        /* disable irq */
> > -       rdev->irq.afmt[dig->afmt->id] = false;
> > -       radeon_irq_set(rdev);
> > +       radeon_irq_kms_disable_afmt(rdev, dig->afmt->id);
> >
> >        /* Older chipsets not handled by AtomBIOS */
> >        if (rdev->family >= CHIP_R600 && !ASIC_IS_DCE3(rdev)) {
> > diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> > index 8479096..23552b4 100644
> > --- a/drivers/gpu/drm/radeon/radeon.h
> > +++ b/drivers/gpu/drm/radeon/radeon.h
> > @@ -610,21 +610,20 @@ union radeon_irq_stat_regs {
> >  #define RADEON_MAX_AFMT_BLOCKS 6
> >
> >  struct radeon_irq {
> > -       bool            installed;
> > -       bool            sw_int[RADEON_NUM_RINGS];
> > -       bool            crtc_vblank_int[RADEON_MAX_CRTCS];
> > -       bool            pflip[RADEON_MAX_CRTCS];
> > -       wait_queue_head_t       vblank_queue;
> > -       bool            hpd[RADEON_MAX_HPD_PINS];
> > -       bool            gui_idle;
> > -       bool            gui_idle_acked;
> > -       wait_queue_head_t       idle_queue;
> > -       bool            afmt[RADEON_MAX_AFMT_BLOCKS];
> > -       spinlock_t sw_lock;
> > -       int sw_refcount[RADEON_NUM_RINGS];
> > -       union radeon_irq_stat_regs stat_regs;
> > -       spinlock_t pflip_lock[RADEON_MAX_CRTCS];
> > -       int pflip_refcount[RADEON_MAX_CRTCS];
> > +       bool                            installed;
> > +       spinlock_t                      lock;
> > +       bool                            sw_int[RADEON_NUM_RINGS];
> > +       int                             sw_refcount[RADEON_NUM_RINGS];
> > +       bool                            crtc_vblank_int[RADEON_MAX_CRTCS];
> > +       bool                            pflip[RADEON_MAX_CRTCS];
> > +       int                             pflip_refcount[RADEON_MAX_CRTCS];
> > +       wait_queue_head_t               vblank_queue;
> > +       bool                            hpd[RADEON_MAX_HPD_PINS];
> > +       bool                            gui_idle;
> > +       bool                            gui_idle_acked;
> > +       wait_queue_head_t               idle_queue;
> > +       bool                            afmt[RADEON_MAX_AFMT_BLOCKS];
> > +       union radeon_irq_stat_regs      stat_regs;
> >  };
> >
> >  int radeon_irq_kms_init(struct radeon_device *rdev);
> > @@ -633,6 +632,9 @@ void radeon_irq_kms_sw_irq_get(struct radeon_device *rdev, int ring);
> >  void radeon_irq_kms_sw_irq_put(struct radeon_device *rdev, int ring);
> >  void radeon_irq_kms_pflip_irq_get(struct radeon_device *rdev, int crtc);
> >  void radeon_irq_kms_pflip_irq_put(struct radeon_device *rdev, int crtc);
> > +void radeon_irq_kms_enable_afmt(struct radeon_device *rdev, int block);
> > +void radeon_irq_kms_disable_afmt(struct radeon_device *rdev, int block);
> > +int radeon_irq_kms_wait_gui_idle(struct radeon_device *rdev);
> >
> >  /*
> >  * CP & rings.
> > @@ -1058,7 +1060,6 @@ struct radeon_pm {
> >        int                     active_crtc_count;
> >        int                     req_vblank;
> >        bool                    vblank_sync;
> > -       bool                    gui_idle;
> >        fixed20_12              max_bandwidth;
> >        fixed20_12              igp_sideport_mclk;
> >        fixed20_12              igp_system_mclk;
> > diff --git a/drivers/gpu/drm/radeon/radeon_irq_kms.c b/drivers/gpu/drm/radeon/radeon_irq_kms.c
> > index 5df58d1..11fc4f7 100644
> > --- a/drivers/gpu/drm/radeon/radeon_irq_kms.c
> > +++ b/drivers/gpu/drm/radeon/radeon_irq_kms.c
> > @@ -32,6 +32,8 @@
> >  #include "radeon.h"
> >  #include "atom.h"
> >
> > +#define RADEON_WAIT_IDLE_TIMEOUT 200
> > +
> >  irqreturn_t radeon_driver_irq_handler_kms(DRM_IRQ_ARGS)
> >  {
> >        struct drm_device *dev = (struct drm_device *) arg;
> > @@ -62,8 +64,10 @@ static void radeon_hotplug_work_func(struct work_struct *work)
> >  void radeon_driver_irq_preinstall_kms(struct drm_device *dev)
> >  {
> >        struct radeon_device *rdev = dev->dev_private;
> > +       unsigned long irqflags;
> >        unsigned i;
> >
> > +       spin_lock_irqsave(&rdev->irq.lock, irqflags);
> >        /* Disable *all* interrupts */
> >        for (i = 0; i < RADEON_NUM_RINGS; i++)
> >                rdev->irq.sw_int[i] = false;
> > @@ -76,6 +80,7 @@ void radeon_driver_irq_preinstall_kms(struct drm_device *dev)
> >                rdev->irq.afmt[i] = false;
> >        }
> >        radeon_irq_set(rdev);
> > +       spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> >        /* Clear bits */
> >        radeon_irq_process(rdev);
> >  }
> > @@ -83,23 +88,28 @@ void radeon_driver_irq_preinstall_kms(struct drm_device *dev)
> >  int radeon_driver_irq_postinstall_kms(struct drm_device *dev)
> >  {
> >        struct radeon_device *rdev = dev->dev_private;
> > +       unsigned long irqflags;
> >        unsigned i;
> >
> >        dev->max_vblank_count = 0x001fffff;
> > +       spin_lock_irqsave(&rdev->irq.lock, irqflags);
> >        for (i = 0; i < RADEON_NUM_RINGS; i++)
> >                rdev->irq.sw_int[i] = true;
> >        radeon_irq_set(rdev);
> > +       spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> >        return 0;
> >  }
> >
> >  void radeon_driver_irq_uninstall_kms(struct drm_device *dev)
> >  {
> >        struct radeon_device *rdev = dev->dev_private;
> > +       unsigned long irqflags;
> >        unsigned i;
> >
> >        if (rdev == NULL) {
> >                return;
> >        }
> > +       spin_lock_irqsave(&rdev->irq.lock, irqflags);
> >        /* Disable *all* interrupts */
> >        for (i = 0; i < RADEON_NUM_RINGS; i++)
> >                rdev->irq.sw_int[i] = false;
> > @@ -112,6 +122,7 @@ void radeon_driver_irq_uninstall_kms(struct drm_device *dev)
> >                rdev->irq.afmt[i] = false;
> >        }
> >        radeon_irq_set(rdev);
> > +       spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> >  }
> >
> >  static bool radeon_msi_ok(struct radeon_device *rdev)
> > @@ -168,15 +179,12 @@ static bool radeon_msi_ok(struct radeon_device *rdev)
> >
> >  int radeon_irq_kms_init(struct radeon_device *rdev)
> >  {
> > -       int i;
> >        int r = 0;
> >
> >        INIT_WORK(&rdev->hotplug_work, radeon_hotplug_work_func);
> >        INIT_WORK(&rdev->audio_work, r600_audio_update_hdmi);
> >
> > -       spin_lock_init(&rdev->irq.sw_lock);
> > -       for (i = 0; i < rdev->num_crtc; i++)
> > -               spin_lock_init(&rdev->irq.pflip_lock[i]);
> > +       spin_lock_init(&rdev->irq.lock);
> >        r = drm_vblank_init(rdev->ddev, rdev->num_crtc);
> >        if (r) {
> >                return r;
> > @@ -217,25 +225,25 @@ void radeon_irq_kms_sw_irq_get(struct radeon_device *rdev, int ring)
> >  {
> >        unsigned long irqflags;
> >
> > -       spin_lock_irqsave(&rdev->irq.sw_lock, irqflags);
> > +       spin_lock_irqsave(&rdev->irq.lock, irqflags);
> >        if (rdev->ddev->irq_enabled && (++rdev->irq.sw_refcount[ring] == 1)) {
> >                rdev->irq.sw_int[ring] = true;
> >                radeon_irq_set(rdev);
> >        }
> > -       spin_unlock_irqrestore(&rdev->irq.sw_lock, irqflags);
> > +       spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> >  }
> >
> >  void radeon_irq_kms_sw_irq_put(struct radeon_device *rdev, int ring)
> >  {
> >        unsigned long irqflags;
> >
> > -       spin_lock_irqsave(&rdev->irq.sw_lock, irqflags);
> > +       spin_lock_irqsave(&rdev->irq.lock, irqflags);
> >        BUG_ON(rdev->ddev->irq_enabled && rdev->irq.sw_refcount[ring] <= 0);
> >        if (rdev->ddev->irq_enabled && (--rdev->irq.sw_refcount[ring] == 0)) {
> >                rdev->irq.sw_int[ring] = false;
> >                radeon_irq_set(rdev);
> >        }
> > -       spin_unlock_irqrestore(&rdev->irq.sw_lock, irqflags);
> > +       spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> >  }
> >
> >  void radeon_irq_kms_pflip_irq_get(struct radeon_device *rdev, int crtc)
> > @@ -245,12 +253,12 @@ void radeon_irq_kms_pflip_irq_get(struct radeon_device *rdev, int crtc)
> >        if (crtc < 0 || crtc >= rdev->num_crtc)
> >                return;
> >
> > -       spin_lock_irqsave(&rdev->irq.pflip_lock[crtc], irqflags);
> > +       spin_lock_irqsave(&rdev->irq.lock, irqflags);
> >        if (rdev->ddev->irq_enabled && (++rdev->irq.pflip_refcount[crtc] == 1)) {
> >                rdev->irq.pflip[crtc] = true;
> >                radeon_irq_set(rdev);
> >        }
> > -       spin_unlock_irqrestore(&rdev->irq.pflip_lock[crtc], irqflags);
> > +       spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> >  }
> >
> >  void radeon_irq_kms_pflip_irq_put(struct radeon_device *rdev, int crtc)
> > @@ -260,12 +268,52 @@ void radeon_irq_kms_pflip_irq_put(struct radeon_device *rdev, int crtc)
> >        if (crtc < 0 || crtc >= rdev->num_crtc)
> >                return;
> >
> > -       spin_lock_irqsave(&rdev->irq.pflip_lock[crtc], irqflags);
> > +       spin_lock_irqsave(&rdev->irq.lock, irqflags);
> >        BUG_ON(rdev->ddev->irq_enabled && rdev->irq.pflip_refcount[crtc] <= 0);
> >        if (rdev->ddev->irq_enabled && (--rdev->irq.pflip_refcount[crtc] == 0)) {
> >                rdev->irq.pflip[crtc] = false;
> >                radeon_irq_set(rdev);
> >        }
> > -       spin_unlock_irqrestore(&rdev->irq.pflip_lock[crtc], irqflags);
> > +       spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> > +}
> > +
> > +void radeon_irq_kms_enable_afmt(struct radeon_device *rdev, int block)
> > +{
> > +       unsigned long irqflags;
> > +
> > +       spin_lock_irqsave(&rdev->irq.lock, irqflags);
> > +       rdev->irq.afmt[block] = true;
> > +       radeon_irq_set(rdev);
> > +       spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> > +
> > +}
> > +
> > +void radeon_irq_kms_disable_afmt(struct radeon_device *rdev, int block)
> > +{
> > +       unsigned long irqflags;
> > +
> > +       spin_lock_irqsave(&rdev->irq.lock, irqflags);
> > +       rdev->irq.afmt[block] = false;
> > +       radeon_irq_set(rdev);
> > +       spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> >  }
> >
> 
> Should probably also add radeon_irq_kms_[en|dis]able_hpd() function
> and call replaced the calls to *_irq_set() in the *_hpd_init() and
> *_hpd_fini() callbacks for display hotplug.
> 
> Alex

Agree, otherwise:
Reviewed-by: Jerome Glisse <jglisse@xxxxxxxxxx>

Cheers,
Jerome

> > +int radeon_irq_kms_wait_gui_idle(struct radeon_device *rdev)
> > +{
> > +       unsigned long irqflags;
> > +       int r;
> > +
> > +       spin_lock_irqsave(&rdev->irq.lock, irqflags);
> > +       rdev->irq.gui_idle = true;
> > +       radeon_irq_set(rdev);
> > +       spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> > +
> > +       r = wait_event_timeout(rdev->irq.idle_queue, radeon_gui_idle(rdev),
> > +                              msecs_to_jiffies(RADEON_WAIT_IDLE_TIMEOUT));
> > +
> > +       spin_lock_irqsave(&rdev->irq.lock, irqflags);
> > +       rdev->irq.gui_idle = false;
> > +       radeon_irq_set(rdev);
> > +       spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> > +       return r;
> > +}
> > diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
> > index f1016a5..abbb04d 100644
> > --- a/drivers/gpu/drm/radeon/radeon_kms.c
> > +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> > @@ -382,29 +382,35 @@ u32 radeon_get_vblank_counter_kms(struct drm_device *dev, int crtc)
> >  int radeon_enable_vblank_kms(struct drm_device *dev, int crtc)
> >  {
> >        struct radeon_device *rdev = dev->dev_private;
> > +       unsigned long irqflags;
> > +       int r;
> >
> >        if (crtc < 0 || crtc >= rdev->num_crtc) {
> >                DRM_ERROR("Invalid crtc %d\n", crtc);
> >                return -EINVAL;
> >        }
> >
> > +       spin_lock_irqsave(&rdev->irq.lock, irqflags);
> >        rdev->irq.crtc_vblank_int[crtc] = true;
> > -
> > -       return radeon_irq_set(rdev);
> > +       r = radeon_irq_set(rdev);
> > +       spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> > +       return r;
> >  }
> >
> >  void radeon_disable_vblank_kms(struct drm_device *dev, int crtc)
> >  {
> >        struct radeon_device *rdev = dev->dev_private;
> > +       unsigned long irqflags;
> >
> >        if (crtc < 0 || crtc >= rdev->num_crtc) {
> >                DRM_ERROR("Invalid crtc %d\n", crtc);
> >                return;
> >        }
> >
> > +       spin_lock_irqsave(&rdev->irq.lock, irqflags);
> >        rdev->irq.crtc_vblank_int[crtc] = false;
> > -
> >        radeon_irq_set(rdev);
> > +       spin_unlock_irqrestore(&rdev->irq.lock, irqflags);
> >  }
> >
> >  int radeon_get_vblank_timestamp_kms(struct drm_device *dev, int crtc,
> > diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
> > index d13b6ae..79642cd 100644
> > --- a/drivers/gpu/drm/radeon/radeon_pm.c
> > +++ b/drivers/gpu/drm/radeon/radeon_pm.c
> > @@ -34,7 +34,6 @@
> >  #define RADEON_IDLE_LOOP_MS 100
> >  #define RADEON_RECLOCK_DELAY_MS 200
> >  #define RADEON_WAIT_VBLANK_TIMEOUT 200
> > -#define RADEON_WAIT_IDLE_TIMEOUT 200
> >
> >  static const char *radeon_pm_state_type_name[5] = {
> >        "Default",
> > @@ -257,15 +256,8 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
> >        /* gui idle int has issues on older chips it seems */
> >        if (rdev->family >= CHIP_R600) {
> >                if (rdev->irq.installed) {
> > -                       /* wait for GPU idle */
> > -                       rdev->pm.gui_idle = false;
> > -                       rdev->irq.gui_idle = true;
> > -                       radeon_irq_set(rdev);
> > -                       wait_event_interruptible_timeout(
> > -                               rdev->irq.idle_queue, rdev->pm.gui_idle,
> > -                               msecs_to_jiffies(RADEON_WAIT_IDLE_TIMEOUT));
> > -                       rdev->irq.gui_idle = false;
> > -                       radeon_irq_set(rdev);
> > +                       /* wait for GPU to become idle */
> > +                       radeon_irq_kms_wait_gui_idle(rdev);
> >                }
> >        } else {
> >                struct radeon_ring *ring = &rdev->ring[RADEON_RING_TYPE_GFX_INDEX];
> > diff --git a/drivers/gpu/drm/radeon/rs600.c b/drivers/gpu/drm/radeon/rs600.c
> > index 25f9eef..4e9c41a 100644
> > --- a/drivers/gpu/drm/radeon/rs600.c
> > +++ b/drivers/gpu/drm/radeon/rs600.c
> > @@ -686,7 +686,6 @@ int rs600_irq_process(struct radeon_device *rdev)
> >                /* GUI idle */
> >                if (G_000040_GUI_IDLE(status)) {
> >                        rdev->irq.gui_idle_acked = true;
> > -                       rdev->pm.gui_idle = true;
> >                        wake_up(&rdev->irq.idle_queue);
> >                }
> >                /* Vertical blank interrupts */
> > diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c
> > index f969487..23be691 100644
> > --- a/drivers/gpu/drm/radeon/si.c
> > +++ b/drivers/gpu/drm/radeon/si.c
> > @@ -3773,7 +3773,6 @@ restart_ih:
> >                        break;
> >                case 233: /* GUI IDLE */
> >                        DRM_DEBUG("IH: GUI idle\n");
> > -                       rdev->pm.gui_idle = true;
> >                        wake_up(&rdev->irq.idle_queue);
> >                        break;
> >                default:
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux