From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Christian_K=F6nig?= Subject: Re: [PATCH 07/10] drm/radeon: apply Murphy's law to the kms irq code Date: Thu, 31 May 2012 22:15:31 +0200 Message-ID: <4FC7D163.9050300@vodafone.de> References: <1337845754-3718-1-git-send-email-deathsimple@vodafone.de> <1337845754-3718-7-git-send-email-deathsimple@vodafone.de> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from outgoing.email.vodafone.de (outgoing.email.vodafone.de [139.7.28.128]) by gabe.freedesktop.org (Postfix) with ESMTP id A3BD89E7C8 for ; Thu, 31 May 2012 13:15:42 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Alex Deucher Cc: Christian Koenig , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On 31.05.2012 20:15, Alex Deucher wrote: > On Thu, May 24, 2012 at 11:35 AM, Alex Deucher wr= ote: >> On Thu, May 24, 2012 at 3:49 AM, Christian K=F6nig >> wrote: >>> From: Christian Koenig >>> >>> 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 >>> --- >>> 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/radeo= n/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 =3D true; >>> wake_up(&rdev->irq.idle_queue); >>> break; >>> default: >>> diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r10= 0..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 =3D true; >>> - rdev->pm.gui_idle =3D true; >>> wake_up(&rdev->irq.idle_queue); >>> } >>> /* Vertical blank interrupts */ >>> diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r60= 0..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 =3D true; >>> wake_up(&rdev->irq.idle_queue); >>> break; >>> default: >>> diff --git a/drivers/gpu/drm/radeon/r600_hdmi.c b/drivers/gpu/drm/radeo= n/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] =3D true; >>> - radeon_irq_set(rdev); >>> + radeon_irq_kms_enable_afmt(rdev, dig->afmt->id); >>> } >>> >>> dig->afmt->enabled =3D 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] =3D false; >>> - radeon_irq_set(rdev); >>> + radeon_irq_kms_disable_afmt(rdev, dig->afmt->id); >>> >>> /* Older chipsets not handled by AtomBIOS */ >>> if (rdev->family>=3D CHIP_R600&& !ASIC_IS_DCE3(rdev)) { >>> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/r= adeon.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_CRTC= S]; >>> + 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 crt= c); >>> void radeon_irq_kms_pflip_irq_put(struct radeon_device *rdev, int crt= c); >>> +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 =3D (struct drm_device *) arg; >>> @@ -62,8 +64,10 @@ static void radeon_hotplug_work_func(struct work_str= uct *work) >>> void radeon_driver_irq_preinstall_kms(struct drm_device *dev) >>> { >>> struct radeon_device *rdev =3D dev->dev_private; >>> + unsigned long irqflags; >>> unsigned i; >>> >>> + spin_lock_irqsave(&rdev->irq.lock, irqflags); >>> /* Disable *all* interrupts */ >>> for (i =3D 0; i< RADEON_NUM_RINGS; i++) >>> rdev->irq.sw_int[i] =3D false; >>> @@ -76,6 +80,7 @@ void radeon_driver_irq_preinstall_kms(struct drm_devi= ce *dev) >>> rdev->irq.afmt[i] =3D 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_de= vice *dev) >>> int radeon_driver_irq_postinstall_kms(struct drm_device *dev) >>> { >>> struct radeon_device *rdev =3D dev->dev_private; >>> + unsigned long irqflags; >>> unsigned i; >>> >>> dev->max_vblank_count =3D 0x001fffff; >>> + spin_lock_irqsave(&rdev->irq.lock, irqflags); >>> for (i =3D 0; i< RADEON_NUM_RINGS; i++) >>> rdev->irq.sw_int[i] =3D 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 =3D dev->dev_private; >>> + unsigned long irqflags; >>> unsigned i; >>> >>> if (rdev =3D=3D NULL) { >>> return; >>> } >>> + spin_lock_irqsave(&rdev->irq.lock, irqflags); >>> /* Disable *all* interrupts */ >>> for (i =3D 0; i< RADEON_NUM_RINGS; i++) >>> rdev->irq.sw_int[i] =3D false; >>> @@ -112,6 +122,7 @@ void radeon_driver_irq_uninstall_kms(struct drm_dev= ice *dev) >>> rdev->irq.afmt[i] =3D 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 *r= dev) >>> >>> int radeon_irq_kms_init(struct radeon_device *rdev) >>> { >>> - int i; >>> int r =3D 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 =3D 0; i< rdev->num_crtc; i++) >>> - spin_lock_init(&rdev->irq.pflip_lock[i]); >>> + spin_lock_init(&rdev->irq.lock); >>> r =3D 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_devi= ce *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] = =3D=3D 1)) { >>> rdev->irq.sw_int[ring] =3D 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]<= =3D 0); >>> if (rdev->ddev->irq_enabled&& (--rdev->irq.sw_refcount[ring] = =3D=3D 0)) { >>> rdev->irq.sw_int[ring] =3D 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 crt= c) >>> @@ -245,12 +253,12 @@ void radeon_irq_kms_pflip_irq_get(struct radeon_d= evice *rdev, int crtc) >>> if (crtc< 0 || crtc>=3D 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= ] =3D=3D 1)) { >>> rdev->irq.pflip[crtc] =3D 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 crt= c) >>> @@ -260,12 +268,52 @@ void radeon_irq_kms_pflip_irq_put(struct radeon_d= evice *rdev, int crtc) >>> if (crtc< 0 || crtc>=3D 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= ]<=3D 0); >>> if (rdev->ddev->irq_enabled&& (--rdev->irq.pflip_refcount[crtc= ] =3D=3D 0)) { >>> rdev->irq.pflip[crtc] =3D 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] =3D 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] =3D 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.