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

Christian König <deathsimple@xxxxxxxxxxx> · Thu, 31 May 2012 22:15:31 +0200

On 31.05.2012 20:15, Alex Deucher wrote:
On Thu, May 24, 2012 at 11:35 AM, Alex Deucher<alexdeucher@xxxxxxxxx>  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.
See attached follow on patch.
The version I pushed into my repo includes nearly the same 
implementation in the v2 version of the patch. I just haven't had time 
to send it to the list yet.

There also is a V2 of the IH patch. After removing the spinlock (and 
with it the disabling of IRQs) in the interrupt handler we seem to hit a 
race condition in the vblank code. Actually I think we can hit the same 
problem when the IH is currently running on one CPU and X modifying 
vblank properties on another CPU, but that is really really really unlikely.

Whatever it is I modified the IH patch to keep the spinlock for now, put 
I think I should look into it more closely.

Anyway going to send out the patches in a minute,
Christian.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel






