On 31.05.2012 20:15, Alex Deucher wrote:
On Thu, May 24, 2012 at 11:35 AM, Alex Deucheralexdeucher@gmail.com wrote:
On Thu, May 24, 2012 at 3:49 AM, Christian König deathsimple@vodafone.de wrote:
From: Christian Koenigchristian.koenig@amd.com
It is really dangerous to have more than one spinlock protecting the same information.
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.
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 Koenigchristian.koenig@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.