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

Alex Deucher alexdeucher at gmail.com
Thu May 31 11:15:08 PDT 2012


On Thu, May 24, 2012 at 11:35 AM, Alex Deucher <alexdeucher at gmail.com> wrote:
> On Thu, May 24, 2012 at 3:49 AM, Christian König
> <deathsimple at vodafone.de> wrote:
>> From: Christian Koenig <christian.koenig at amd.com>
>>
>> 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 at amd.com>
>> ---
>>  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.

Alex
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-drm-radeon-apply-Murphy-s-law-to-the-kms-hpd-irq-cod.patch
Type: text/x-patch
Size: 13299 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20120531/2386658a/attachment-0001.bin>


More information about the dri-devel mailing list