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

[Christian König]

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.

v2: Also handle the hpd irq code the same way.
v3: Rename hpd parameter for clarification.

Signed-off-by: Christian Koenig 
Reviewed-by: Alex Deucher 
---
 drivers/gpu/drm/radeon/evergreen.c  |   21 ++-
 drivers/gpu/drm/radeon/r100.c   |   29 ++
 drivers/gpu/drm/radeon/r600.c   |   38 
 drivers/gpu/drm/radeon/r600_hdmi.c  |6 +-
 drivers/gpu/drm/radeon/radeon.h |   35 +--
 drivers/gpu/drm/radeon/radeon_irq_kms.c |   96 +++
 drivers/gpu/drm/radeon/radeon_kms.c |   12 +++-
 drivers/gpu/drm/radeon/radeon_pm.c  |   12 +---
 drivers/gpu/drm/radeon/rs600.c  |   13 ++---
 drivers/gpu/drm/radeon/si.c |1 -
 10 files changed, 144 insertions(+), 119 deletions(-)

diff --git a/drivers/gpu/drm/radeon/evergreen.c 
b/drivers/gpu/drm/radeon/evergreen.c
index 18abe3c..8b64227 100644
--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -428,6 +428,7 @@ void evergreen_hpd_init(struct radeon_device *rdev)
 {
struct drm_device *dev = rdev->ddev;
struct drm_connector *connector;
+   unsigned enabled = 0;
u32 tmp = DC_HPDx_CONNECTION_TIMER(0x9c4) |
DC_HPDx_RX_INT_TIMER(0xfa) | DC_HPDx_EN;

@@ -436,73 +437,64 @@ void evergreen_hpd_init(struct radeon_device *rdev)
switch (radeon_connector->hpd.hpd) {
case RADEON_HPD_1:
WREG32(DC_HPD1_CONTROL, tmp);
-   rdev->irq.hpd[0] = true;
break;
case RADEON_HPD_2:
WREG32(DC_HPD2_CONTROL, tmp);
-   rdev->irq.hpd[1] = true;
break;
case RADEON_HPD_3:
WREG32(DC_HPD3_CONTROL, tmp);
-   rdev->irq.hpd[2] = true;
break;
case RADEON_HPD_4:
WREG32(DC_HPD4_CONTROL, tmp);
-   rdev->irq.hpd[3] = true;
break;
case RADEON_HPD_5:
WREG32(DC_HPD5_CONTROL, tmp);
-   rdev->irq.hpd[4] = true;
break;
case RADEON_HPD_6:
WREG32(DC_HPD6_CONTROL, tmp);
-   rdev->irq.hpd[5] = true;
break;
default:
break;
}
radeon_hpd_set_polarity(rdev, radeon_connector->hpd.hpd);
+   enabled |= 1 << radeon_connector->hpd.hpd;
}
-   if (rdev->irq.installed)
-   evergreen_irq_set(rdev);
+   radeon_irq_kms_enable_hpd(rdev, enabled);
 }

 void evergreen_hpd_fini(struct radeon_device *rdev)
 {
struct drm_device *dev = rdev->ddev;
struct drm_connector *connector;
+   unsigned disabled = 0;

list_for_each_entry(connector, >mode_config.connector_list, head) {
struct radeon_connector *radeon_connector = 
to_radeon_connector(connector);
switch (radeon_connector->hpd.hpd) {
case RADEON_HPD_1:
WREG32(DC_HPD1_CONTROL, 0);
-   rdev->irq.hpd[0] = false;
break;
case RADEON_HPD_2:
WREG32(DC_HPD2_CONTROL, 0);
-   rdev->irq.hpd[1] = false;
break;
case RADEON_HPD_3:
WREG32(DC_HPD3_CONTROL, 0);
-   rdev->irq.hpd[2] = false;
break;
case RADEON_HPD_4:
WREG32(DC_HPD4_CONTROL, 0);
-   rdev->irq.hpd[3] = false;
break;
case RADEON_HPD_5:
WREG32(DC_HPD5_CONTROL, 0);
-   rdev->irq.hpd[4] = false;
break;
case RADEON_HPD_6:
WREG32(DC_HPD6_CONTROL, 0);
-   rdev->irq.hpd[5] = false;
break;
default:
break;
}
+   disabled |= 1 << radeon_connector->hpd.hpd;
}
+   radeon_irq_kms_disable_hpd(rdev, disabled);
 }

 /* watermark setup */
@@ -3251,7 +3243,6 @@ restart_ih:
break;
case 233: /* 

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

[Christian König]

From: Christian Koenig christian.koe...@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.

v2: Also handle the hpd irq code the same way.
v3: Rename hpd parameter for clarification.

Signed-off-by: Christian Koenig christian.koe...@amd.com
Reviewed-by: Alex Deucher alexander.deuc...@amd.com
---
 drivers/gpu/drm/radeon/evergreen.c  |   21 ++-
 drivers/gpu/drm/radeon/r100.c   |   29 ++
 drivers/gpu/drm/radeon/r600.c   |   38 
 drivers/gpu/drm/radeon/r600_hdmi.c  |6 +-
 drivers/gpu/drm/radeon/radeon.h |   35 +--
 drivers/gpu/drm/radeon/radeon_irq_kms.c |   96 +++
 drivers/gpu/drm/radeon/radeon_kms.c |   12 +++-
 drivers/gpu/drm/radeon/radeon_pm.c  |   12 +---
 drivers/gpu/drm/radeon/rs600.c  |   13 ++---
 drivers/gpu/drm/radeon/si.c |1 -
 10 files changed, 144 insertions(+), 119 deletions(-)

diff --git a/drivers/gpu/drm/radeon/evergreen.c 
b/drivers/gpu/drm/radeon/evergreen.c
index 18abe3c..8b64227 100644
--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -428,6 +428,7 @@ void evergreen_hpd_init(struct radeon_device *rdev)
 {
struct drm_device *dev = rdev-ddev;
struct drm_connector *connector;
+   unsigned enabled = 0;
u32 tmp = DC_HPDx_CONNECTION_TIMER(0x9c4) |
DC_HPDx_RX_INT_TIMER(0xfa) | DC_HPDx_EN;
 
@@ -436,73 +437,64 @@ void evergreen_hpd_init(struct radeon_device *rdev)
switch (radeon_connector-hpd.hpd) {
case RADEON_HPD_1:
WREG32(DC_HPD1_CONTROL, tmp);
-   rdev-irq.hpd[0] = true;
break;
case RADEON_HPD_2:
WREG32(DC_HPD2_CONTROL, tmp);
-   rdev-irq.hpd[1] = true;
break;
case RADEON_HPD_3:
WREG32(DC_HPD3_CONTROL, tmp);
-   rdev-irq.hpd[2] = true;
break;
case RADEON_HPD_4:
WREG32(DC_HPD4_CONTROL, tmp);
-   rdev-irq.hpd[3] = true;
break;
case RADEON_HPD_5:
WREG32(DC_HPD5_CONTROL, tmp);
-   rdev-irq.hpd[4] = true;
break;
case RADEON_HPD_6:
WREG32(DC_HPD6_CONTROL, tmp);
-   rdev-irq.hpd[5] = true;
break;
default:
break;
}
radeon_hpd_set_polarity(rdev, radeon_connector-hpd.hpd);
+   enabled |= 1  radeon_connector-hpd.hpd;
}
-   if (rdev-irq.installed)
-   evergreen_irq_set(rdev);
+   radeon_irq_kms_enable_hpd(rdev, enabled);
 }
 
 void evergreen_hpd_fini(struct radeon_device *rdev)
 {
struct drm_device *dev = rdev-ddev;
struct drm_connector *connector;
+   unsigned disabled = 0;
 
list_for_each_entry(connector, dev-mode_config.connector_list, head) {
struct radeon_connector *radeon_connector = 
to_radeon_connector(connector);
switch (radeon_connector-hpd.hpd) {
case RADEON_HPD_1:
WREG32(DC_HPD1_CONTROL, 0);
-   rdev-irq.hpd[0] = false;
break;
case RADEON_HPD_2:
WREG32(DC_HPD2_CONTROL, 0);
-   rdev-irq.hpd[1] = false;
break;
case RADEON_HPD_3:
WREG32(DC_HPD3_CONTROL, 0);
-   rdev-irq.hpd[2] = false;
break;
case RADEON_HPD_4:
WREG32(DC_HPD4_CONTROL, 0);
-   rdev-irq.hpd[3] = false;
break;
case RADEON_HPD_5:
WREG32(DC_HPD5_CONTROL, 0);
-   rdev-irq.hpd[4] = false;
break;
case RADEON_HPD_6:
WREG32(DC_HPD6_CONTROL, 0);
-   rdev-irq.hpd[5] = false;
break;
default:
break;
}
+   disabled |= 1  radeon_connector-hpd.hpd;
}
+   radeon_irq_kms_disable_hpd(rdev, disabled);
 }
 
 /* watermark setup */
@@ -3251,7 +3243,6 @@ restart_ih:

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

[Christian König]

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.

v2: Also handle the hpd irq code the same way.

Signed-off-by: Christian Koenig 
---
 drivers/gpu/drm/radeon/evergreen.c  |   21 ++-
 drivers/gpu/drm/radeon/r100.c   |   29 ++
 drivers/gpu/drm/radeon/r600.c   |   38 
 drivers/gpu/drm/radeon/r600_hdmi.c  |6 +-
 drivers/gpu/drm/radeon/radeon.h |   35 +--
 drivers/gpu/drm/radeon/radeon_irq_kms.c |   96 +++
 drivers/gpu/drm/radeon/radeon_kms.c |   12 +++-
 drivers/gpu/drm/radeon/radeon_pm.c  |   12 +---
 drivers/gpu/drm/radeon/rs600.c  |   13 ++---
 drivers/gpu/drm/radeon/si.c |1 -
 10 files changed, 144 insertions(+), 119 deletions(-)

diff --git a/drivers/gpu/drm/radeon/evergreen.c 
b/drivers/gpu/drm/radeon/evergreen.c
index e4c3321..48ec1a0 100644
--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -428,6 +428,7 @@ void evergreen_hpd_init(struct radeon_device *rdev)
 {
struct drm_device *dev = rdev->ddev;
struct drm_connector *connector;
+   unsigned enabled = 0;
u32 tmp = DC_HPDx_CONNECTION_TIMER(0x9c4) |
DC_HPDx_RX_INT_TIMER(0xfa) | DC_HPDx_EN;

@@ -436,73 +437,64 @@ void evergreen_hpd_init(struct radeon_device *rdev)
switch (radeon_connector->hpd.hpd) {
case RADEON_HPD_1:
WREG32(DC_HPD1_CONTROL, tmp);
-   rdev->irq.hpd[0] = true;
break;
case RADEON_HPD_2:
WREG32(DC_HPD2_CONTROL, tmp);
-   rdev->irq.hpd[1] = true;
break;
case RADEON_HPD_3:
WREG32(DC_HPD3_CONTROL, tmp);
-   rdev->irq.hpd[2] = true;
break;
case RADEON_HPD_4:
WREG32(DC_HPD4_CONTROL, tmp);
-   rdev->irq.hpd[3] = true;
break;
case RADEON_HPD_5:
WREG32(DC_HPD5_CONTROL, tmp);
-   rdev->irq.hpd[4] = true;
break;
case RADEON_HPD_6:
WREG32(DC_HPD6_CONTROL, tmp);
-   rdev->irq.hpd[5] = true;
break;
default:
break;
}
radeon_hpd_set_polarity(rdev, radeon_connector->hpd.hpd);
+   enabled |= 1 << radeon_connector->hpd.hpd;
}
-   if (rdev->irq.installed)
-   evergreen_irq_set(rdev);
+   radeon_irq_kms_enable_hpd(rdev, enabled);
 }

 void evergreen_hpd_fini(struct radeon_device *rdev)
 {
struct drm_device *dev = rdev->ddev;
struct drm_connector *connector;
+   unsigned disabled = 0;

list_for_each_entry(connector, >mode_config.connector_list, head) {
struct radeon_connector *radeon_connector = 
to_radeon_connector(connector);
switch (radeon_connector->hpd.hpd) {
case RADEON_HPD_1:
WREG32(DC_HPD1_CONTROL, 0);
-   rdev->irq.hpd[0] = false;
break;
case RADEON_HPD_2:
WREG32(DC_HPD2_CONTROL, 0);
-   rdev->irq.hpd[1] = false;
break;
case RADEON_HPD_3:
WREG32(DC_HPD3_CONTROL, 0);
-   rdev->irq.hpd[2] = false;
break;
case RADEON_HPD_4:
WREG32(DC_HPD4_CONTROL, 0);
-   rdev->irq.hpd[3] = false;
break;
case RADEON_HPD_5:
WREG32(DC_HPD5_CONTROL, 0);
-   rdev->irq.hpd[4] = false;
break;
case RADEON_HPD_6:
WREG32(DC_HPD6_CONTROL, 0);
-   rdev->irq.hpd[5] = false;
break;
default:
break;
}
+   disabled |= 1 << radeon_connector->hpd.hpd;
}
+   radeon_irq_kms_disable_hpd(rdev, disabled);
 }

 /* watermark setup */
@@ -3252,7 +3244,6 @@ restart_ih:
break;
case 233: /* GUI IDLE */
DRM_DEBUG("IH: GUI idle\n");
- 

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

[Christian König]

On 31.05.2012 20:15, Alex Deucher wrote:
> On Thu, May 24, 2012 at 11:35 AM, Alex Deucher  
> wrote:
>> On Thu, May 24, 2012 at 3:49 AM, Christian K?nig
>>   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/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(>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(>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(>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 {
>>> -   boolinstalled;
>>> -   boolsw_int[RADEON_NUM_RINGS];
>>> -   boolcrtc_vblank_int[RADEON_MAX_CRTCS];
>>> -   boolpflip[RADEON_MAX_CRTCS];
>>> -   wait_queue_head_t   vblank_queue;
>>> -   boolhpd[RADEON_MAX_HPD_PINS];
>>> -   boolgui_idle;
>>> -   boolgui_idle_acked;
>>> -   wait_queue_head_t   idle_queue;
>>> -   bool

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

[Alex Deucher]

On Thu, May 31, 2012 at 4:16 PM, Christian K?nig
 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.
>
> v2: Also handle the hpd irq code the same way.

couple of comments below for clarity, other than that:

Reviewed-by: Alex Deucher 

>
> Signed-off-by: Christian Koenig 
> ---
> ?drivers/gpu/drm/radeon/evergreen.c ? ? ?| ? 21 ++-
> ?drivers/gpu/drm/radeon/r100.c ? ? ? ? ? | ? 29 ++
> ?drivers/gpu/drm/radeon/r600.c ? ? ? ? ? | ? 38 
> ?drivers/gpu/drm/radeon/r600_hdmi.c ? ? ?| ? ?6 +-
> ?drivers/gpu/drm/radeon/radeon.h ? ? ? ? | ? 35 +--
> ?drivers/gpu/drm/radeon/radeon_irq_kms.c | ? 96 
> +++
> ?drivers/gpu/drm/radeon/radeon_kms.c ? ? | ? 12 +++-
> ?drivers/gpu/drm/radeon/radeon_pm.c ? ? ?| ? 12 +---
> ?drivers/gpu/drm/radeon/rs600.c ? ? ? ? ?| ? 13 ++---
> ?drivers/gpu/drm/radeon/si.c ? ? ? ? ? ? | ? ?1 -
> ?10 files changed, 144 insertions(+), 119 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/evergreen.c 
> b/drivers/gpu/drm/radeon/evergreen.c
> index e4c3321..48ec1a0 100644
> --- a/drivers/gpu/drm/radeon/evergreen.c
> +++ b/drivers/gpu/drm/radeon/evergreen.c
> @@ -428,6 +428,7 @@ void evergreen_hpd_init(struct radeon_device *rdev)
> ?{
> ? ? ? ?struct drm_device *dev = rdev->ddev;
> ? ? ? ?struct drm_connector *connector;
> + ? ? ? unsigned enabled = 0;
> ? ? ? ?u32 tmp = DC_HPDx_CONNECTION_TIMER(0x9c4) |
> ? ? ? ? ? ? ? ?DC_HPDx_RX_INT_TIMER(0xfa) | DC_HPDx_EN;
>
> @@ -436,73 +437,64 @@ void evergreen_hpd_init(struct radeon_device *rdev)
> ? ? ? ? ? ? ? ?switch (radeon_connector->hpd.hpd) {
> ? ? ? ? ? ? ? ?case RADEON_HPD_1:
> ? ? ? ? ? ? ? ? ? ? ? ?WREG32(DC_HPD1_CONTROL, tmp);
> - ? ? ? ? ? ? ? ? ? ? ? rdev->irq.hpd[0] = true;
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? ?case RADEON_HPD_2:
> ? ? ? ? ? ? ? ? ? ? ? ?WREG32(DC_HPD2_CONTROL, tmp);
> - ? ? ? ? ? ? ? ? ? ? ? rdev->irq.hpd[1] = true;
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? ?case RADEON_HPD_3:
> ? ? ? ? ? ? ? ? ? ? ? ?WREG32(DC_HPD3_CONTROL, tmp);
> - ? ? ? ? ? ? ? ? ? ? ? rdev->irq.hpd[2] = true;
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? ?case RADEON_HPD_4:
> ? ? ? ? ? ? ? ? ? ? ? ?WREG32(DC_HPD4_CONTROL, tmp);
> - ? ? ? ? ? ? ? ? ? ? ? rdev->irq.hpd[3] = true;
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? ?case RADEON_HPD_5:
> ? ? ? ? ? ? ? ? ? ? ? ?WREG32(DC_HPD5_CONTROL, tmp);
> - ? ? ? ? ? ? ? ? ? ? ? rdev->irq.hpd[4] = true;
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? ?case RADEON_HPD_6:
> ? ? ? ? ? ? ? ? ? ? ? ?WREG32(DC_HPD6_CONTROL, tmp);
> - ? ? ? ? ? ? ? ? ? ? ? rdev->irq.hpd[5] = true;
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? ?default:
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? ?}
> ? ? ? ? ? ? ? ?radeon_hpd_set_polarity(rdev, radeon_connector->hpd.hpd);
> + ? ? ? ? ? ? ? enabled |= 1 << radeon_connector->hpd.hpd;
> ? ? ? ?}
> - ? ? ? if (rdev->irq.installed)
> - ? ? ? ? ? ? ? evergreen_irq_set(rdev);
> + ? ? ? radeon_irq_kms_enable_hpd(rdev, enabled);
> ?}
>
> ?void evergreen_hpd_fini(struct radeon_device *rdev)
> ?{
> ? ? ? ?struct drm_device *dev = rdev->ddev;
> ? ? ? ?struct drm_connector *connector;
> + ? ? ? unsigned disabled = 0;
>
> ? ? ? ?list_for_each_entry(connector, >mode_config.connector_list, head) 
> {
> ? ? ? ? ? ? ? ?struct radeon_connector *radeon_connector = 
> to_radeon_connector(connector);
> ? ? ? ? ? ? ? ?switch (radeon_connector->hpd.hpd) {
> ? ? ? ? ? ? ? ?case RADEON_HPD_1:
> ? ? ? ? ? ? ? ? ? ? ? ?WREG32(DC_HPD1_CONTROL, 0);
> - ? ? ? ? ? ? ? ? ? ? ? rdev->irq.hpd[0] = false;
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? ?case RADEON_HPD_2:
> ? ? ? ? ? ? ? ? ? ? ? ?WREG32(DC_HPD2_CONTROL, 0);
> - ? ? ? ? ? ? ? ? ? ? ? rdev->irq.hpd[1] = false;
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? ?case RADEON_HPD_3:
> ? ? ? ? ? ? ? ? ? ? ? ?WREG32(DC_HPD3_CONTROL, 0);
> - ? ? ? ? ? ? ? ? ? ? ? rdev->irq.hpd[2] = false;
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? ?case RADEON_HPD_4:
> ? ? ? ? ? ? ? ? ? ? ? ?WREG32(DC_HPD4_CONTROL, 0);
> - ? ? ? ? ? ? ? ? ? ? ? rdev->irq.hpd[3] = false;
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? ?case RADEON_HPD_5:
> ? ? ? ? ? ? ? ? ? ? ? ?WREG32(DC_HPD5_CONTROL, 0);
> - ? ? ? ? ? ? ? ? ? ? ? rdev->irq.hpd[4] = false;
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? ?case RADEON_HPD_6:
> ? ? ? ? ? ? ? ? ? ? ? ?WREG32(DC_HPD6_CONTROL, 0);
> - ? ? ? ? ? ? ? ? ? ? ? rdev->irq.hpd[5] = false;
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? ?default:
> ? ? ? ? ? ? ? ? ? ? ? ?break;
> ? ? ? ? ? ? ? ?}
> + ? ? ? ? ? ? ? 

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

[Jerome Glisse]

On Thu, May 31, 2012 at 2:15 PM, Alex Deucher  wrote:
> On Thu, May 24, 2012 at 11:35 AM, Alex Deucher  
> wrote:
>> On Thu, May 24, 2012 at 3:49 AM, Christian K?nig
>>  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/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(>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(>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(>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];
>>> - ? ? 

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

[Alex Deucher]

On Thu, May 24, 2012 at 11:35 AM, Alex Deucher  wrote:
> On Thu, May 24, 2012 at 3:49 AM, Christian K?nig
>  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/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(>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(>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(>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];
>> 

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

[Alex Deucher]

On Thu, May 24, 2012 at 11:35 AM, Alex Deucher alexdeuc...@gmail.com wrote:
 On Thu, May 24, 2012 at 3:49 AM, Christian König
 deathsim...@vodafone.de wrote:
 From: Christian Koenig christian.koe...@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.koe...@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;
 +       

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

[Jerome Glisse]

On Thu, May 31, 2012 at 2:15 PM, Alex Deucher alexdeuc...@gmail.com wrote:
 On Thu, May 24, 2012 at 11:35 AM, Alex Deucher alexdeuc...@gmail.com wrote:
 On Thu, May 24, 2012 at 3:49 AM, Christian König
 deathsim...@vodafone.de wrote:
 From: Christian Koenig christian.koe...@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.koe...@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                       

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

[Christian König]

On 31.05.2012 20:15, Alex Deucher wrote:

On Thu, May 24, 2012 at 11:35 AM, Alex Deucheralexdeuc...@gmail.com  wrote:

On Thu, May 24, 2012 at 3:49 AM, Christian König
deathsim...@vodafone.de  wrote:

From: Christian Koenigchristian.koe...@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 Koenigchristian.koe...@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 {
-   boolinstalled;
-   boolsw_int[RADEON_NUM_RINGS];
-   boolcrtc_vblank_int[RADEON_MAX_CRTCS];
-   boolpflip[RADEON_MAX_CRTCS];
-   wait_queue_head_t   vblank_queue;
-   boolhpd[RADEON_MAX_HPD_PINS];
-   boolgui_idle;
-   boolgui_idle_acked;
-   wait_queue_head_t   idle_queue;
-   boolafmt[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];
+   boolinstalled;
+   spinlock_t  lock;
+   bool  

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

[Christian König]

From: Christian Koenig christian.koe...@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.

v2: Also handle the hpd irq code the same way.

Signed-off-by: Christian Koenig christian.koe...@amd.com
---
 drivers/gpu/drm/radeon/evergreen.c  |   21 ++-
 drivers/gpu/drm/radeon/r100.c   |   29 ++
 drivers/gpu/drm/radeon/r600.c   |   38 
 drivers/gpu/drm/radeon/r600_hdmi.c  |6 +-
 drivers/gpu/drm/radeon/radeon.h |   35 +--
 drivers/gpu/drm/radeon/radeon_irq_kms.c |   96 +++
 drivers/gpu/drm/radeon/radeon_kms.c |   12 +++-
 drivers/gpu/drm/radeon/radeon_pm.c  |   12 +---
 drivers/gpu/drm/radeon/rs600.c  |   13 ++---
 drivers/gpu/drm/radeon/si.c |1 -
 10 files changed, 144 insertions(+), 119 deletions(-)

diff --git a/drivers/gpu/drm/radeon/evergreen.c 
b/drivers/gpu/drm/radeon/evergreen.c
index e4c3321..48ec1a0 100644
--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -428,6 +428,7 @@ void evergreen_hpd_init(struct radeon_device *rdev)
 {
struct drm_device *dev = rdev-ddev;
struct drm_connector *connector;
+   unsigned enabled = 0;
u32 tmp = DC_HPDx_CONNECTION_TIMER(0x9c4) |
DC_HPDx_RX_INT_TIMER(0xfa) | DC_HPDx_EN;
 
@@ -436,73 +437,64 @@ void evergreen_hpd_init(struct radeon_device *rdev)
switch (radeon_connector-hpd.hpd) {
case RADEON_HPD_1:
WREG32(DC_HPD1_CONTROL, tmp);
-   rdev-irq.hpd[0] = true;
break;
case RADEON_HPD_2:
WREG32(DC_HPD2_CONTROL, tmp);
-   rdev-irq.hpd[1] = true;
break;
case RADEON_HPD_3:
WREG32(DC_HPD3_CONTROL, tmp);
-   rdev-irq.hpd[2] = true;
break;
case RADEON_HPD_4:
WREG32(DC_HPD4_CONTROL, tmp);
-   rdev-irq.hpd[3] = true;
break;
case RADEON_HPD_5:
WREG32(DC_HPD5_CONTROL, tmp);
-   rdev-irq.hpd[4] = true;
break;
case RADEON_HPD_6:
WREG32(DC_HPD6_CONTROL, tmp);
-   rdev-irq.hpd[5] = true;
break;
default:
break;
}
radeon_hpd_set_polarity(rdev, radeon_connector-hpd.hpd);
+   enabled |= 1  radeon_connector-hpd.hpd;
}
-   if (rdev-irq.installed)
-   evergreen_irq_set(rdev);
+   radeon_irq_kms_enable_hpd(rdev, enabled);
 }
 
 void evergreen_hpd_fini(struct radeon_device *rdev)
 {
struct drm_device *dev = rdev-ddev;
struct drm_connector *connector;
+   unsigned disabled = 0;
 
list_for_each_entry(connector, dev-mode_config.connector_list, head) {
struct radeon_connector *radeon_connector = 
to_radeon_connector(connector);
switch (radeon_connector-hpd.hpd) {
case RADEON_HPD_1:
WREG32(DC_HPD1_CONTROL, 0);
-   rdev-irq.hpd[0] = false;
break;
case RADEON_HPD_2:
WREG32(DC_HPD2_CONTROL, 0);
-   rdev-irq.hpd[1] = false;
break;
case RADEON_HPD_3:
WREG32(DC_HPD3_CONTROL, 0);
-   rdev-irq.hpd[2] = false;
break;
case RADEON_HPD_4:
WREG32(DC_HPD4_CONTROL, 0);
-   rdev-irq.hpd[3] = false;
break;
case RADEON_HPD_5:
WREG32(DC_HPD5_CONTROL, 0);
-   rdev-irq.hpd[4] = false;
break;
case RADEON_HPD_6:
WREG32(DC_HPD6_CONTROL, 0);
-   rdev-irq.hpd[5] = false;
break;
default:
break;
}
+   disabled |= 1  radeon_connector-hpd.hpd;
}
+   radeon_irq_kms_disable_hpd(rdev, disabled);
 }
 
 /* watermark setup */
@@ -3252,7 +3244,6 @@ restart_ih:
break;
case 233: /* GUI IDLE */
DRM_DEBUG(IH: GUI idle\n);
-  

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

[j.glisse]

On Thu, May 24, 2012 at 11:35:15AM -0400, Alex Deucher wrote:
> On Thu, May 24, 2012 at 3:49 AM, Christian K?nig
>  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/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(>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(>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(>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 

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

[Alex Deucher]

On Thu, May 24, 2012 at 3:49 AM, Christian K?nig
 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/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(>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(>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(>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 ? ? ? ? ? ? ? ? 

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

[Christian König]

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/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(>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(>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(>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 {
-   boolinstalled;
-   boolsw_int[RADEON_NUM_RINGS];
-   boolcrtc_vblank_int[RADEON_MAX_CRTCS];
-   boolpflip[RADEON_MAX_CRTCS];
-   wait_queue_head_t   vblank_queue;
-   boolhpd[RADEON_MAX_HPD_PINS];
-   boolgui_idle;
-   boolgui_idle_acked;
-   wait_queue_head_t   idle_queue;
-   boolafmt[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];
+   boolinstalled;
+   spinlock_t  lock;
+   boolsw_int[RADEON_NUM_RINGS];
+   int sw_refcount[RADEON_NUM_RINGS];
+   boolcrtc_vblank_int[RADEON_MAX_CRTCS];
+   bool

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

[Christian König]

From: Christian Koenig christian.koe...@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.koe...@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 {
-   boolinstalled;
-   boolsw_int[RADEON_NUM_RINGS];
-   boolcrtc_vblank_int[RADEON_MAX_CRTCS];
-   boolpflip[RADEON_MAX_CRTCS];
-   wait_queue_head_t   vblank_queue;
-   boolhpd[RADEON_MAX_HPD_PINS];
-   boolgui_idle;
-   boolgui_idle_acked;
-   wait_queue_head_t   idle_queue;
-   boolafmt[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];
+   boolinstalled;
+   spinlock_t  lock;
+   boolsw_int[RADEON_NUM_RINGS];
+   int sw_refcount[RADEON_NUM_RINGS];
+   boolcrtc_vblank_int[RADEON_MAX_CRTCS];
+   bool   

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

[Alex Deucher]

On Thu, May 24, 2012 at 3:49 AM, Christian König
deathsim...@vodafone.de wrote:
 From: Christian Koenig christian.koe...@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.koe...@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              

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

[j.glisse]

On Thu, May 24, 2012 at 11:35:15AM -0400, Alex Deucher wrote:
 On Thu, May 24, 2012 at 3:49 AM, Christian König
 deathsim...@vodafone.de wrote:
  From: Christian Koenig christian.koe...@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.koe...@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];
  +