[PATCH 07/10] drm/radeon: apply Murphy's law to the kms irq code
Christian König
deathsimple at vodafone.de
Thu May 31 13:15:31 PDT 2012
On 31.05.2012 20:15, Alex Deucher wrote:
> 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.
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.
More information about the dri-devel
mailing list