[PATCH 2/2] nouveau: Acknowledge HPD irq in handler, not bottom half
On Wed, 2010-11-10 at 18:01 -0500, Andrew Lutomirski wrote: > On Wed, Nov 10, 2010 at 5:55 PM, Maarten Maathuis > wrote: > > On Wed, Nov 10, 2010 at 11:51 PM, Andrew Lutomirski wrote: > >> On Wed, Nov 10, 2010 at 5:35 PM, Ben Skeggs wrote: > >>> On Wed, 2010-11-10 at 17:25 -0500, Andrew Lutomirski wrote: > On Wed, Nov 10, 2010 at 5:10 PM, Ben Skeggs > wrote: > > On Wed, 2010-11-10 at 16:32 -0500, Andy Lutomirski wrote: > >> The old code generated an interrupt storm bad enough to completely > >> take down my system. > >> > >> This only fixes the bits that are defined nouveau_regs.h. Newer > >> hardware > >> uses another register that isn't described, and I don't have that > >> hardware > >> to test. > > Thanks for looking at this. I'll take a closer look at the problem > > today and see what I can come up with too, that'll work with the newer > > hardware too. > > It should be as simple as adding an hpd1 field to the hpd_state and > making exactly the same change. (It would be nice to put the register > definitions into nouveau_regs.h as well -- I didn't really want to > muck around with a bunch of magic numbers that I can't test.) > >>> Yes, it is. I can confirm the problem on another card, but it doesn't > >>> actually cause any crashes here. If you can rework the patch to support > >>> the newer chips too, that'd be great. > >>> > >>> As for magic numbers, the register names for those regs are wrong > >>> anyway. The joy of reverse-engineering the support. It doesn't really > >>> matter if you want to stick to them or go back to "magic" numbers. > >> > >> That explains why INTR and CTRL seemed backwards :) I'll leave the > >> magic numbers for the 0xe07? stuff. > > > > Perhaps remove the bad definitions from the reg file, or rename them > > to UNKsomething? > > Well, they're known. One is hotplug detect enable (unless the code is > wrong) and the other is hotplug interrupt status. That's also not correct, if anything the most accurate names so far would probably be: #define NV_PGPIO_INTR_EN_0 0xe050 #define NV_PGPIO_INTR_00xe054 #define NV_PGPIO_INTR_EN_1 0xe070 #define NV_PGPIO_INTR_10xe074 PGPIO is a guess, and there's other stuff in that range too, but it's definitely *not* PCONNECTOR. Anyway, this doesn't matter. Whatever change in names can happen in nouveau git and make it's way to Linus from there, the fix for nouveau git is already going to be different enough from what'll apply on Linus' tree right now. My opinion is, lets just fix the bug in mainline (without register naming) and fix the naming etc in nouveau git. Ben. > > > > --Andy
[PATCH 2/2] nouveau: Acknowledge HPD irq in handler, not bottom half
On Wed, 2010-11-10 at 17:51 -0500, Andrew Lutomirski wrote: > On Wed, Nov 10, 2010 at 5:35 PM, Ben Skeggs wrote: > > On Wed, 2010-11-10 at 17:25 -0500, Andrew Lutomirski wrote: > >> On Wed, Nov 10, 2010 at 5:10 PM, Ben Skeggs wrote: > >> > On Wed, 2010-11-10 at 16:32 -0500, Andy Lutomirski wrote: > >> >> The old code generated an interrupt storm bad enough to completely > >> >> take down my system. > >> >> > >> >> This only fixes the bits that are defined nouveau_regs.h. Newer > >> >> hardware > >> >> uses another register that isn't described, and I don't have that > >> >> hardware > >> >> to test. > >> > Thanks for looking at this. I'll take a closer look at the problem > >> > today and see what I can come up with too, that'll work with the newer > >> > hardware too. > >> > >> It should be as simple as adding an hpd1 field to the hpd_state and > >> making exactly the same change. (It would be nice to put the register > >> definitions into nouveau_regs.h as well -- I didn't really want to > >> muck around with a bunch of magic numbers that I can't test.) > > Yes, it is. I can confirm the problem on another card, but it doesn't > > actually cause any crashes here. If you can rework the patch to support > > the newer chips too, that'd be great. > > > > As for magic numbers, the register names for those regs are wrong > > anyway. The joy of reverse-engineering the support. It doesn't really > > matter if you want to stick to them or go back to "magic" numbers. > > That explains why INTR and CTRL seemed backwards :) I'll leave the > magic numbers for the 0xe07? stuff. That sounds good, it'll all get a cleanup at some point and switched to "proper" (well, our best guess, you'd have to ask NVIDIA about the real ones) names. Ben. > > Also, I accidentally dropped the "& enabled_bits" part -- I'll put that back. > > Patch to follow after I boot and test it here. > > --Andy
[PATCH 2/2] nouveau: Acknowledge HPD irq in handler, not bottom half
On Wed, 2010-11-10 at 17:25 -0500, Andrew Lutomirski wrote:
> On Wed, Nov 10, 2010 at 5:10 PM, Ben Skeggs wrote:
> > On Wed, 2010-11-10 at 16:32 -0500, Andy Lutomirski wrote:
> >> The old code generated an interrupt storm bad enough to completely
> >> take down my system.
> >>
> >> This only fixes the bits that are defined nouveau_regs.h. Newer hardware
> >> uses another register that isn't described, and I don't have that hardware
> >> to test.
> > Thanks for looking at this. I'll take a closer look at the problem
> > today and see what I can come up with too, that'll work with the newer
> > hardware too.
>
> It should be as simple as adding an hpd1 field to the hpd_state and
> making exactly the same change. (It would be nice to put the register
> definitions into nouveau_regs.h as well -- I didn't really want to
> muck around with a bunch of magic numbers that I can't test.)
Yes, it is. I can confirm the problem on another card, but it doesn't
actually cause any crashes here. If you can rework the patch to support
the newer chips too, that'd be great.
As for magic numbers, the register names for those regs are wrong
anyway. The joy of reverse-engineering the support. It doesn't really
matter if you want to stick to them or go back to "magic" numbers.
Ben.
>
> I tried writing 0x to the display IRQ control in the handler
> to explicitly acknowledge the IRQ, but either I did it wrong or it had
> no effect.
>
> I imagine that this explains the unreproducible crashes I had on F13 as well.
>
> --Andy
>
> >
> > Ben.
> >>
> >> Signed-off-by: Andy Lutomirski
> >> Cc:
> >> ---
> >> drivers/gpu/drm/nouveau/nouveau_drv.h |5 +
> >> drivers/gpu/drm/nouveau/nouveau_irq.c |1 +
> >> drivers/gpu/drm/nouveau/nv50_display.c | 17 +
> >> 3 files changed, 19 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
> >> b/drivers/gpu/drm/nouveau/nouveau_drv.h
> >> index b1be617..b6c62cc 100644
> >> --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> >> +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> >> @@ -531,6 +531,11 @@ struct drm_nouveau_private {
> >> struct work_struct irq_work;
> >> struct work_struct hpd_work;
> >>
> >> + struct {
> >> + spinlock_t lock;
> >> + uint32_t hpd0_bits;
> >> + } hpd_state;
> >> +
> >> struct list_head vbl_waiting;
> >>
> >> struct {
> >> diff --git a/drivers/gpu/drm/nouveau/nouveau_irq.c
> >> b/drivers/gpu/drm/nouveau/nouveau_irq.c
> >> index 794b0ee..b62a601 100644
> >> --- a/drivers/gpu/drm/nouveau/nouveau_irq.c
> >> +++ b/drivers/gpu/drm/nouveau/nouveau_irq.c
> >> @@ -52,6 +52,7 @@ nouveau_irq_preinstall(struct drm_device *dev)
> >> if (dev_priv->card_type >= NV_50) {
> >> INIT_WORK(_priv->irq_work, nv50_display_irq_handler_bh);
> >> INIT_WORK(_priv->hpd_work, nv50_display_irq_hotplug_bh);
> >> + spin_lock_init(_priv->hpd_state.lock);
> >> INIT_LIST_HEAD(_priv->vbl_waiting);
> >> }
> >> }
> >> diff --git a/drivers/gpu/drm/nouveau/nv50_display.c
> >> b/drivers/gpu/drm/nouveau/nv50_display.c
> >> index 83a7d27..0df08e3 100644
> >> --- a/drivers/gpu/drm/nouveau/nv50_display.c
> >> +++ b/drivers/gpu/drm/nouveau/nv50_display.c
> >> @@ -1014,7 +1014,12 @@ nv50_display_irq_hotplug_bh(struct work_struct
> >> *work)
> >> uint32_t unplug_mask, plug_mask, change_mask;
> >> uint32_t hpd0, hpd1 = 0;
> >>
> >> - hpd0 = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL) & nv_rd32(dev,
> >> NV50_PCONNECTOR_HOTPLUG_INTR);
> >> + spin_lock_irq(_priv->hpd_state.lock);
> >> + hpd0 = dev_priv->hpd_state.hpd0_bits;
> >> + dev_priv->hpd_state.hpd0_bits = 0;
> >> + spin_unlock_irq(_priv->hpd_state.lock);
> >> +
> >> + hpd0 &= nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_INTR);
> >> if (dev_priv->chipset >= 0x90)
> >> hpd1 = nv_rd32(dev, 0xe074) & nv_rd32(dev, 0xe070);
> >>
> >> @@ -1058,7 +1063,6 @@ nv50_display_irq_hotplug_bh(struct work_struct *work)
> >> helper->dpms(connector->encoder, DRM_MODE_DPMS_OFF);
> >> }
> >>
> >> - nv_wr32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL, nv_rd32(dev,
> >> NV50_PCONNECTOR_HOTPLUG_CTRL));
> >> if (dev_priv->chipset >= 0x90)
> >> nv_wr32(dev, 0xe074, nv_rd32(dev, 0xe074));
> >>
> >> @@ -1072,8 +1076,13 @@ nv50_display_irq_handler(struct drm_device *dev)
> >> uint32_t delayed = 0;
> >>
> >> if (nv_rd32(dev, NV50_PMC_INTR_0) & NV50_PMC_INTR_0_HOTPLUG) {
> >> - if (!work_pending(_priv->hpd_work))
> >> - queue_work(dev_priv->wq, _priv->hpd_work);
> >> + uint32_t hpd0_bits = nv_rd32(dev,
> >> NV50_PCONNECTOR_HOTPLUG_CTRL);
> >> + nv_wr32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL, hpd0_bits);
> >> + spin_lock(_priv->hpd_state.lock);
> >> + dev_priv->hpd_state.hpd0_bits |= hpd0_bits;
> >> +
[PATCH 2/2] nouveau: Acknowledge HPD irq in handler, not bottom half
On Wed, 2010-11-10 at 16:32 -0500, Andy Lutomirski wrote:
> The old code generated an interrupt storm bad enough to completely
> take down my system.
>
> This only fixes the bits that are defined nouveau_regs.h. Newer hardware
> uses another register that isn't described, and I don't have that hardware
> to test.
Thanks for looking at this. I'll take a closer look at the problem
today and see what I can come up with too, that'll work with the newer
hardware too.
Ben.
>
> Signed-off-by: Andy Lutomirski
> Cc:
> ---
> drivers/gpu/drm/nouveau/nouveau_drv.h |5 +
> drivers/gpu/drm/nouveau/nouveau_irq.c |1 +
> drivers/gpu/drm/nouveau/nv50_display.c | 17 +
> 3 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
> b/drivers/gpu/drm/nouveau/nouveau_drv.h
> index b1be617..b6c62cc 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> @@ -531,6 +531,11 @@ struct drm_nouveau_private {
> struct work_struct irq_work;
> struct work_struct hpd_work;
>
> + struct {
> + spinlock_t lock;
> + uint32_t hpd0_bits;
> + } hpd_state;
> +
> struct list_head vbl_waiting;
>
> struct {
> diff --git a/drivers/gpu/drm/nouveau/nouveau_irq.c
> b/drivers/gpu/drm/nouveau/nouveau_irq.c
> index 794b0ee..b62a601 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_irq.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_irq.c
> @@ -52,6 +52,7 @@ nouveau_irq_preinstall(struct drm_device *dev)
> if (dev_priv->card_type >= NV_50) {
> INIT_WORK(_priv->irq_work, nv50_display_irq_handler_bh);
> INIT_WORK(_priv->hpd_work, nv50_display_irq_hotplug_bh);
> + spin_lock_init(_priv->hpd_state.lock);
> INIT_LIST_HEAD(_priv->vbl_waiting);
> }
> }
> diff --git a/drivers/gpu/drm/nouveau/nv50_display.c
> b/drivers/gpu/drm/nouveau/nv50_display.c
> index 83a7d27..0df08e3 100644
> --- a/drivers/gpu/drm/nouveau/nv50_display.c
> +++ b/drivers/gpu/drm/nouveau/nv50_display.c
> @@ -1014,7 +1014,12 @@ nv50_display_irq_hotplug_bh(struct work_struct *work)
> uint32_t unplug_mask, plug_mask, change_mask;
> uint32_t hpd0, hpd1 = 0;
>
> - hpd0 = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL) & nv_rd32(dev,
> NV50_PCONNECTOR_HOTPLUG_INTR);
> + spin_lock_irq(_priv->hpd_state.lock);
> + hpd0 = dev_priv->hpd_state.hpd0_bits;
> + dev_priv->hpd_state.hpd0_bits = 0;
> + spin_unlock_irq(_priv->hpd_state.lock);
> +
> + hpd0 &= nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_INTR);
> if (dev_priv->chipset >= 0x90)
> hpd1 = nv_rd32(dev, 0xe074) & nv_rd32(dev, 0xe070);
>
> @@ -1058,7 +1063,6 @@ nv50_display_irq_hotplug_bh(struct work_struct *work)
> helper->dpms(connector->encoder, DRM_MODE_DPMS_OFF);
> }
>
> - nv_wr32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL, nv_rd32(dev,
> NV50_PCONNECTOR_HOTPLUG_CTRL));
> if (dev_priv->chipset >= 0x90)
> nv_wr32(dev, 0xe074, nv_rd32(dev, 0xe074));
>
> @@ -1072,8 +1076,13 @@ nv50_display_irq_handler(struct drm_device *dev)
> uint32_t delayed = 0;
>
> if (nv_rd32(dev, NV50_PMC_INTR_0) & NV50_PMC_INTR_0_HOTPLUG) {
> - if (!work_pending(_priv->hpd_work))
> - queue_work(dev_priv->wq, _priv->hpd_work);
> + uint32_t hpd0_bits = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL);
> + nv_wr32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL, hpd0_bits);
> + spin_lock(_priv->hpd_state.lock);
> + dev_priv->hpd_state.hpd0_bits |= hpd0_bits;
> + spin_unlock(_priv->hpd_state.lock);
> +
> + queue_work(dev_priv->wq, _priv->hpd_work);
> }
>
> while (nv_rd32(dev, NV50_PMC_INTR_0) & NV50_PMC_INTR_0_DISPLAY) {
[PATCH 2/2] nouveau: Acknowledge HPD irq in handler, not bottom half
On Wed, Nov 10, 2010 at 11:51 PM, Andrew Lutomirski wrote: > On Wed, Nov 10, 2010 at 5:35 PM, Ben Skeggs wrote: >> On Wed, 2010-11-10 at 17:25 -0500, Andrew Lutomirski wrote: >>> On Wed, Nov 10, 2010 at 5:10 PM, Ben Skeggs wrote: >>> > On Wed, 2010-11-10 at 16:32 -0500, Andy Lutomirski wrote: >>> >> The old code generated an interrupt storm bad enough to completely >>> >> take down my system. >>> >> >>> >> This only fixes the bits that are defined nouveau_regs.h. ?Newer hardware >>> >> uses another register that isn't described, and I don't have that >>> >> hardware >>> >> to test. >>> > Thanks for looking at this. ?I'll take a closer look at the problem >>> > today and see what I can come up with too, that'll work with the newer >>> > hardware too. >>> >>> It should be as simple as adding an hpd1 field to the hpd_state and >>> making exactly the same change. ?(It would be nice to put the register >>> definitions into nouveau_regs.h as well -- I didn't really want to >>> muck around with a bunch of magic numbers that I can't test.) >> Yes, it is. ?I can confirm the problem on another card, but it doesn't >> actually cause any crashes here. ?If you can rework the patch to support >> the newer chips too, that'd be great. >> >> As for magic numbers, the register names for those regs are wrong >> anyway. ?The joy of reverse-engineering the support. ?It doesn't really >> matter if you want to stick to them or go back to "magic" numbers. > > That explains why INTR and CTRL seemed backwards :) ?I'll leave the > magic numbers for the 0xe07? stuff. Perhaps remove the bad definitions from the reg file, or rename them to UNKsomething? > > Also, I accidentally dropped the "& enabled_bits" part -- I'll put that back. > > Patch to follow after I boot and test it here. > > --Andy > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Far away from the primal instinct, the song seems to fade away, the river get wider between your thoughts and the things we do and say.
[PATCH 2/2] nouveau: Acknowledge HPD irq in handler, not bottom half
The old code generated an interrupt storm bad enough to completely
take down my system.
Signed-off-by: Andy Lutomirski
---
drivers/gpu/drm/nouveau/nouveau_drv.h |6 +
drivers/gpu/drm/nouveau/nouveau_irq.c |1 +
drivers/gpu/drm/nouveau/nv50_display.c | 35 +++
3 files changed, 33 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
b/drivers/gpu/drm/nouveau/nouveau_drv.h
index b1be617..c926d88 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -531,6 +531,12 @@ struct drm_nouveau_private {
struct work_struct irq_work;
struct work_struct hpd_work;
+ struct {
+ spinlock_t lock;
+ uint32_t hpd0_bits;
+ uint32_t hpd1_bits;
+ } hpd_state;
+
struct list_head vbl_waiting;
struct {
diff --git a/drivers/gpu/drm/nouveau/nouveau_irq.c
b/drivers/gpu/drm/nouveau/nouveau_irq.c
index 794b0ee..b62a601 100644
--- a/drivers/gpu/drm/nouveau/nouveau_irq.c
+++ b/drivers/gpu/drm/nouveau/nouveau_irq.c
@@ -52,6 +52,7 @@ nouveau_irq_preinstall(struct drm_device *dev)
if (dev_priv->card_type >= NV_50) {
INIT_WORK(_priv->irq_work, nv50_display_irq_handler_bh);
INIT_WORK(_priv->hpd_work, nv50_display_irq_hotplug_bh);
+ spin_lock_init(_priv->hpd_state.lock);
INIT_LIST_HEAD(_priv->vbl_waiting);
}
}
diff --git a/drivers/gpu/drm/nouveau/nv50_display.c
b/drivers/gpu/drm/nouveau/nv50_display.c
index 83a7d27..014f69c 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -1012,11 +1012,18 @@ nv50_display_irq_hotplug_bh(struct work_struct *work)
struct drm_connector *connector;
const uint32_t gpio_reg[4] = { 0xe104, 0xe108, 0xe280, 0xe284 };
uint32_t unplug_mask, plug_mask, change_mask;
- uint32_t hpd0, hpd1 = 0;
+ uint32_t hpd0, hpd1;
- hpd0 = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL) & nv_rd32(dev,
NV50_PCONNECTOR_HOTPLUG_INTR);
+ spin_lock_irq(_priv->hpd_state.lock);
+ hpd0 = dev_priv->hpd_state.hpd0_bits;
+ dev_priv->hpd_state.hpd0_bits = 0;
+ hpd1 = dev_priv->hpd_state.hpd1_bits;
+ dev_priv->hpd_state.hpd1_bits = 0;
+ spin_unlock_irq(_priv->hpd_state.lock);
+
+ hpd0 &= nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_INTR);
if (dev_priv->chipset >= 0x90)
- hpd1 = nv_rd32(dev, 0xe074) & nv_rd32(dev, 0xe070);
+ hpd1 &= nv_rd32(dev, 0xe070);
plug_mask = (hpd0 & 0x) | (hpd1 << 16);
unplug_mask = (hpd0 >> 16) | (hpd1 & 0x);
@@ -1058,10 +1065,6 @@ nv50_display_irq_hotplug_bh(struct work_struct *work)
helper->dpms(connector->encoder, DRM_MODE_DPMS_OFF);
}
- nv_wr32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL, nv_rd32(dev,
NV50_PCONNECTOR_HOTPLUG_CTRL));
- if (dev_priv->chipset >= 0x90)
- nv_wr32(dev, 0xe074, nv_rd32(dev, 0xe074));
-
drm_helper_hpd_irq_event(dev);
}
@@ -1072,8 +1075,22 @@ nv50_display_irq_handler(struct drm_device *dev)
uint32_t delayed = 0;
if (nv_rd32(dev, NV50_PMC_INTR_0) & NV50_PMC_INTR_0_HOTPLUG) {
- if (!work_pending(_priv->hpd_work))
- queue_work(dev_priv->wq, _priv->hpd_work);
+ uint32_t hpd0_bits, hpd1_bits = 0;
+
+ hpd0_bits = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL);
+ nv_wr32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL, hpd0_bits);
+
+ if (dev_priv->chipset >= 0x90) {
+ hpd1_bits = nv_rd32(dev, 0xe074);
+ nv_wr32(dev, 0xe074, hpd1_bits);
+ }
+
+ spin_lock(_priv->hpd_state.lock);
+ dev_priv->hpd_state.hpd0_bits |= hpd0_bits;
+ dev_priv->hpd_state.hpd1_bits |= hpd1_bits;
+ spin_unlock(_priv->hpd_state.lock);
+
+ queue_work(dev_priv->wq, _priv->hpd_work);
}
while (nv_rd32(dev, NV50_PMC_INTR_0) & NV50_PMC_INTR_0_DISPLAY) {
--
1.7.3.2
[PATCH 2/2] nouveau: Acknowledge HPD irq in handler, not bottom half
On Wed, Nov 10, 2010 at 5:55 PM, Maarten Maathuis wrote: > On Wed, Nov 10, 2010 at 11:51 PM, Andrew Lutomirski wrote: >> On Wed, Nov 10, 2010 at 5:35 PM, Ben Skeggs wrote: >>> On Wed, 2010-11-10 at 17:25 -0500, Andrew Lutomirski wrote: On Wed, Nov 10, 2010 at 5:10 PM, Ben Skeggs wrote: > On Wed, 2010-11-10 at 16:32 -0500, Andy Lutomirski wrote: >> The old code generated an interrupt storm bad enough to completely >> take down my system. >> >> This only fixes the bits that are defined nouveau_regs.h. ?Newer >> hardware >> uses another register that isn't described, and I don't have that >> hardware >> to test. > Thanks for looking at this. ?I'll take a closer look at the problem > today and see what I can come up with too, that'll work with the newer > hardware too. It should be as simple as adding an hpd1 field to the hpd_state and making exactly the same change. ?(It would be nice to put the register definitions into nouveau_regs.h as well -- I didn't really want to muck around with a bunch of magic numbers that I can't test.) >>> Yes, it is. ?I can confirm the problem on another card, but it doesn't >>> actually cause any crashes here. ?If you can rework the patch to support >>> the newer chips too, that'd be great. >>> >>> As for magic numbers, the register names for those regs are wrong >>> anyway. ?The joy of reverse-engineering the support. ?It doesn't really >>> matter if you want to stick to them or go back to "magic" numbers. >> >> That explains why INTR and CTRL seemed backwards :) ?I'll leave the >> magic numbers for the 0xe07? stuff. > > Perhaps remove the bad definitions from the reg file, or rename them > to UNKsomething? Well, they're known. One is hotplug detect enable (unless the code is wrong) and the other is hotplug interrupt status. --Andy
[PATCH 2/2] nouveau: Acknowledge HPD irq in handler, not bottom half
On Wed, Nov 10, 2010 at 5:35 PM, Ben Skeggs wrote: > On Wed, 2010-11-10 at 17:25 -0500, Andrew Lutomirski wrote: >> On Wed, Nov 10, 2010 at 5:10 PM, Ben Skeggs wrote: >> > On Wed, 2010-11-10 at 16:32 -0500, Andy Lutomirski wrote: >> >> The old code generated an interrupt storm bad enough to completely >> >> take down my system. >> >> >> >> This only fixes the bits that are defined nouveau_regs.h. ?Newer hardware >> >> uses another register that isn't described, and I don't have that hardware >> >> to test. >> > Thanks for looking at this. ?I'll take a closer look at the problem >> > today and see what I can come up with too, that'll work with the newer >> > hardware too. >> >> It should be as simple as adding an hpd1 field to the hpd_state and >> making exactly the same change. ?(It would be nice to put the register >> definitions into nouveau_regs.h as well -- I didn't really want to >> muck around with a bunch of magic numbers that I can't test.) > Yes, it is. ?I can confirm the problem on another card, but it doesn't > actually cause any crashes here. ?If you can rework the patch to support > the newer chips too, that'd be great. > > As for magic numbers, the register names for those regs are wrong > anyway. ?The joy of reverse-engineering the support. ?It doesn't really > matter if you want to stick to them or go back to "magic" numbers. That explains why INTR and CTRL seemed backwards :) I'll leave the magic numbers for the 0xe07? stuff. Also, I accidentally dropped the "& enabled_bits" part -- I'll put that back. Patch to follow after I boot and test it here. --Andy
[PATCH 2/2] nouveau: Acknowledge HPD irq in handler, not bottom half
On Wed, Nov 10, 2010 at 5:10 PM, Ben Skeggs wrote:
> On Wed, 2010-11-10 at 16:32 -0500, Andy Lutomirski wrote:
>> The old code generated an interrupt storm bad enough to completely
>> take down my system.
>>
>> This only fixes the bits that are defined nouveau_regs.h. ?Newer hardware
>> uses another register that isn't described, and I don't have that hardware
>> to test.
> Thanks for looking at this. ?I'll take a closer look at the problem
> today and see what I can come up with too, that'll work with the newer
> hardware too.
It should be as simple as adding an hpd1 field to the hpd_state and
making exactly the same change. (It would be nice to put the register
definitions into nouveau_regs.h as well -- I didn't really want to
muck around with a bunch of magic numbers that I can't test.)
I tried writing 0x to the display IRQ control in the handler
to explicitly acknowledge the IRQ, but either I did it wrong or it had
no effect.
I imagine that this explains the unreproducible crashes I had on F13 as well.
--Andy
>
> Ben.
>>
>> Signed-off-by: Andy Lutomirski
>> Cc:
>> ---
>> ?drivers/gpu/drm/nouveau/nouveau_drv.h ?| ? ?5 +
>> ?drivers/gpu/drm/nouveau/nouveau_irq.c ?| ? ?1 +
>> ?drivers/gpu/drm/nouveau/nv50_display.c | ? 17 +
>> ?3 files changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
>> b/drivers/gpu/drm/nouveau/nouveau_drv.h
>> index b1be617..b6c62cc 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
>> +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
>> @@ -531,6 +531,11 @@ struct drm_nouveau_private {
>> ? ? ? struct work_struct irq_work;
>> ? ? ? struct work_struct hpd_work;
>>
>> + ? ? struct {
>> + ? ? ? ? ? ? spinlock_t lock;
>> + ? ? ? ? ? ? uint32_t hpd0_bits;
>> + ? ? } hpd_state;
>> +
>> ? ? ? struct list_head vbl_waiting;
>>
>> ? ? ? struct {
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_irq.c
>> b/drivers/gpu/drm/nouveau/nouveau_irq.c
>> index 794b0ee..b62a601 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_irq.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_irq.c
>> @@ -52,6 +52,7 @@ nouveau_irq_preinstall(struct drm_device *dev)
>> ? ? ? if (dev_priv->card_type >= NV_50) {
>> ? ? ? ? ? ? ? INIT_WORK(_priv->irq_work, nv50_display_irq_handler_bh);
>> ? ? ? ? ? ? ? INIT_WORK(_priv->hpd_work, nv50_display_irq_hotplug_bh);
>> + ? ? ? ? ? ? spin_lock_init(_priv->hpd_state.lock);
>> ? ? ? ? ? ? ? INIT_LIST_HEAD(_priv->vbl_waiting);
>> ? ? ? }
>> ?}
>> diff --git a/drivers/gpu/drm/nouveau/nv50_display.c
>> b/drivers/gpu/drm/nouveau/nv50_display.c
>> index 83a7d27..0df08e3 100644
>> --- a/drivers/gpu/drm/nouveau/nv50_display.c
>> +++ b/drivers/gpu/drm/nouveau/nv50_display.c
>> @@ -1014,7 +1014,12 @@ nv50_display_irq_hotplug_bh(struct work_struct *work)
>> ? ? ? uint32_t unplug_mask, plug_mask, change_mask;
>> ? ? ? uint32_t hpd0, hpd1 = 0;
>>
>> - ? ? hpd0 = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL) & nv_rd32(dev,
>> NV50_PCONNECTOR_HOTPLUG_INTR);
>> + ? ? spin_lock_irq(_priv->hpd_state.lock);
>> + ? ? hpd0 = dev_priv->hpd_state.hpd0_bits;
>> + ? ? dev_priv->hpd_state.hpd0_bits = 0;
>> + ? ? spin_unlock_irq(_priv->hpd_state.lock);
>> +
>> + ? ? hpd0 &= nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_INTR);
>> ? ? ? if (dev_priv->chipset >= 0x90)
>> ? ? ? ? ? ? ? hpd1 = nv_rd32(dev, 0xe074) & nv_rd32(dev, 0xe070);
>>
>> @@ -1058,7 +1063,6 @@ nv50_display_irq_hotplug_bh(struct work_struct *work)
>> ? ? ? ? ? ? ? ? ? ? ? helper->dpms(connector->encoder, DRM_MODE_DPMS_OFF);
>> ? ? ? }
>>
>> - ? ? nv_wr32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL, nv_rd32(dev,
>> NV50_PCONNECTOR_HOTPLUG_CTRL));
>> ? ? ? if (dev_priv->chipset >= 0x90)
>> ? ? ? ? ? ? ? nv_wr32(dev, 0xe074, nv_rd32(dev, 0xe074));
>>
>> @@ -1072,8 +1076,13 @@ nv50_display_irq_handler(struct drm_device *dev)
>> ? ? ? uint32_t delayed = 0;
>>
>> ? ? ? if (nv_rd32(dev, NV50_PMC_INTR_0) & NV50_PMC_INTR_0_HOTPLUG) {
>> - ? ? ? ? ? ? if (!work_pending(_priv->hpd_work))
>> - ? ? ? ? ? ? ? ? ? ? queue_work(dev_priv->wq, _priv->hpd_work);
>> + ? ? ? ? ? ? uint32_t hpd0_bits = nv_rd32(dev,
>> NV50_PCONNECTOR_HOTPLUG_CTRL);
>> + ? ? ? ? ? ? nv_wr32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL, hpd0_bits);
>> + ? ? ? ? ? ? spin_lock(_priv->hpd_state.lock);
>> + ? ? ? ? ? ? dev_priv->hpd_state.hpd0_bits |= hpd0_bits;
>> + ? ? ? ? ? ? spin_unlock(_priv->hpd_state.lock);
>> +
>> + ? ? ? ? ? ? queue_work(dev_priv->wq, _priv->hpd_work);
>> ? ? ? }
>>
>> ? ? ? while (nv_rd32(dev, NV50_PMC_INTR_0) & NV50_PMC_INTR_0_DISPLAY) {
>
>
>
[PATCH 2/2] nouveau: Acknowledge HPD irq in handler, not bottom half
The old code generated an interrupt storm bad enough to completely
take down my system.
This only fixes the bits that are defined nouveau_regs.h. Newer hardware
uses another register that isn't described, and I don't have that hardware
to test.
Signed-off-by: Andy Lutomirski
Cc:
---
drivers/gpu/drm/nouveau/nouveau_drv.h |5 +
drivers/gpu/drm/nouveau/nouveau_irq.c |1 +
drivers/gpu/drm/nouveau/nv50_display.c | 17 +
3 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
b/drivers/gpu/drm/nouveau/nouveau_drv.h
index b1be617..b6c62cc 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -531,6 +531,11 @@ struct drm_nouveau_private {
struct work_struct irq_work;
struct work_struct hpd_work;
+ struct {
+ spinlock_t lock;
+ uint32_t hpd0_bits;
+ } hpd_state;
+
struct list_head vbl_waiting;
struct {
diff --git a/drivers/gpu/drm/nouveau/nouveau_irq.c
b/drivers/gpu/drm/nouveau/nouveau_irq.c
index 794b0ee..b62a601 100644
--- a/drivers/gpu/drm/nouveau/nouveau_irq.c
+++ b/drivers/gpu/drm/nouveau/nouveau_irq.c
@@ -52,6 +52,7 @@ nouveau_irq_preinstall(struct drm_device *dev)
if (dev_priv->card_type >= NV_50) {
INIT_WORK(_priv->irq_work, nv50_display_irq_handler_bh);
INIT_WORK(_priv->hpd_work, nv50_display_irq_hotplug_bh);
+ spin_lock_init(_priv->hpd_state.lock);
INIT_LIST_HEAD(_priv->vbl_waiting);
}
}
diff --git a/drivers/gpu/drm/nouveau/nv50_display.c
b/drivers/gpu/drm/nouveau/nv50_display.c
index 83a7d27..0df08e3 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -1014,7 +1014,12 @@ nv50_display_irq_hotplug_bh(struct work_struct *work)
uint32_t unplug_mask, plug_mask, change_mask;
uint32_t hpd0, hpd1 = 0;
- hpd0 = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL) & nv_rd32(dev,
NV50_PCONNECTOR_HOTPLUG_INTR);
+ spin_lock_irq(_priv->hpd_state.lock);
+ hpd0 = dev_priv->hpd_state.hpd0_bits;
+ dev_priv->hpd_state.hpd0_bits = 0;
+ spin_unlock_irq(_priv->hpd_state.lock);
+
+ hpd0 &= nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_INTR);
if (dev_priv->chipset >= 0x90)
hpd1 = nv_rd32(dev, 0xe074) & nv_rd32(dev, 0xe070);
@@ -1058,7 +1063,6 @@ nv50_display_irq_hotplug_bh(struct work_struct *work)
helper->dpms(connector->encoder, DRM_MODE_DPMS_OFF);
}
- nv_wr32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL, nv_rd32(dev,
NV50_PCONNECTOR_HOTPLUG_CTRL));
if (dev_priv->chipset >= 0x90)
nv_wr32(dev, 0xe074, nv_rd32(dev, 0xe074));
@@ -1072,8 +1076,13 @@ nv50_display_irq_handler(struct drm_device *dev)
uint32_t delayed = 0;
if (nv_rd32(dev, NV50_PMC_INTR_0) & NV50_PMC_INTR_0_HOTPLUG) {
- if (!work_pending(_priv->hpd_work))
- queue_work(dev_priv->wq, _priv->hpd_work);
+ uint32_t hpd0_bits = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL);
+ nv_wr32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL, hpd0_bits);
+ spin_lock(_priv->hpd_state.lock);
+ dev_priv->hpd_state.hpd0_bits |= hpd0_bits;
+ spin_unlock(_priv->hpd_state.lock);
+
+ queue_work(dev_priv->wq, _priv->hpd_work);
}
while (nv_rd32(dev, NV50_PMC_INTR_0) & NV50_PMC_INTR_0_DISPLAY) {
--
1.7.3.2
[PATCH 2/2] nouveau: Acknowledge HPD irq in handler, not bottom half
The old code generated an interrupt storm bad enough to completely
take down my system.
This only fixes the bits that are defined nouveau_regs.h. Newer hardware
uses another register that isn't described, and I don't have that hardware
to test.
Signed-off-by: Andy Lutomirski l...@mit.edu
Cc: sta...@kernel.org
---
drivers/gpu/drm/nouveau/nouveau_drv.h |5 +
drivers/gpu/drm/nouveau/nouveau_irq.c |1 +
drivers/gpu/drm/nouveau/nv50_display.c | 17 +
3 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
b/drivers/gpu/drm/nouveau/nouveau_drv.h
index b1be617..b6c62cc 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -531,6 +531,11 @@ struct drm_nouveau_private {
struct work_struct irq_work;
struct work_struct hpd_work;
+ struct {
+ spinlock_t lock;
+ uint32_t hpd0_bits;
+ } hpd_state;
+
struct list_head vbl_waiting;
struct {
diff --git a/drivers/gpu/drm/nouveau/nouveau_irq.c
b/drivers/gpu/drm/nouveau/nouveau_irq.c
index 794b0ee..b62a601 100644
--- a/drivers/gpu/drm/nouveau/nouveau_irq.c
+++ b/drivers/gpu/drm/nouveau/nouveau_irq.c
@@ -52,6 +52,7 @@ nouveau_irq_preinstall(struct drm_device *dev)
if (dev_priv-card_type = NV_50) {
INIT_WORK(dev_priv-irq_work, nv50_display_irq_handler_bh);
INIT_WORK(dev_priv-hpd_work, nv50_display_irq_hotplug_bh);
+ spin_lock_init(dev_priv-hpd_state.lock);
INIT_LIST_HEAD(dev_priv-vbl_waiting);
}
}
diff --git a/drivers/gpu/drm/nouveau/nv50_display.c
b/drivers/gpu/drm/nouveau/nv50_display.c
index 83a7d27..0df08e3 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -1014,7 +1014,12 @@ nv50_display_irq_hotplug_bh(struct work_struct *work)
uint32_t unplug_mask, plug_mask, change_mask;
uint32_t hpd0, hpd1 = 0;
- hpd0 = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL) nv_rd32(dev,
NV50_PCONNECTOR_HOTPLUG_INTR);
+ spin_lock_irq(dev_priv-hpd_state.lock);
+ hpd0 = dev_priv-hpd_state.hpd0_bits;
+ dev_priv-hpd_state.hpd0_bits = 0;
+ spin_unlock_irq(dev_priv-hpd_state.lock);
+
+ hpd0 = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_INTR);
if (dev_priv-chipset = 0x90)
hpd1 = nv_rd32(dev, 0xe074) nv_rd32(dev, 0xe070);
@@ -1058,7 +1063,6 @@ nv50_display_irq_hotplug_bh(struct work_struct *work)
helper-dpms(connector-encoder, DRM_MODE_DPMS_OFF);
}
- nv_wr32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL, nv_rd32(dev,
NV50_PCONNECTOR_HOTPLUG_CTRL));
if (dev_priv-chipset = 0x90)
nv_wr32(dev, 0xe074, nv_rd32(dev, 0xe074));
@@ -1072,8 +1076,13 @@ nv50_display_irq_handler(struct drm_device *dev)
uint32_t delayed = 0;
if (nv_rd32(dev, NV50_PMC_INTR_0) NV50_PMC_INTR_0_HOTPLUG) {
- if (!work_pending(dev_priv-hpd_work))
- queue_work(dev_priv-wq, dev_priv-hpd_work);
+ uint32_t hpd0_bits = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL);
+ nv_wr32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL, hpd0_bits);
+ spin_lock(dev_priv-hpd_state.lock);
+ dev_priv-hpd_state.hpd0_bits |= hpd0_bits;
+ spin_unlock(dev_priv-hpd_state.lock);
+
+ queue_work(dev_priv-wq, dev_priv-hpd_work);
}
while (nv_rd32(dev, NV50_PMC_INTR_0) NV50_PMC_INTR_0_DISPLAY) {
--
1.7.3.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] nouveau: Acknowledge HPD irq in handler, not bottom half
On Wed, 2010-11-10 at 16:32 -0500, Andy Lutomirski wrote:
The old code generated an interrupt storm bad enough to completely
take down my system.
This only fixes the bits that are defined nouveau_regs.h. Newer hardware
uses another register that isn't described, and I don't have that hardware
to test.
Thanks for looking at this. I'll take a closer look at the problem
today and see what I can come up with too, that'll work with the newer
hardware too.
Ben.
Signed-off-by: Andy Lutomirski l...@mit.edu
Cc: sta...@kernel.org
---
drivers/gpu/drm/nouveau/nouveau_drv.h |5 +
drivers/gpu/drm/nouveau/nouveau_irq.c |1 +
drivers/gpu/drm/nouveau/nv50_display.c | 17 +
3 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
b/drivers/gpu/drm/nouveau/nouveau_drv.h
index b1be617..b6c62cc 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -531,6 +531,11 @@ struct drm_nouveau_private {
struct work_struct irq_work;
struct work_struct hpd_work;
+ struct {
+ spinlock_t lock;
+ uint32_t hpd0_bits;
+ } hpd_state;
+
struct list_head vbl_waiting;
struct {
diff --git a/drivers/gpu/drm/nouveau/nouveau_irq.c
b/drivers/gpu/drm/nouveau/nouveau_irq.c
index 794b0ee..b62a601 100644
--- a/drivers/gpu/drm/nouveau/nouveau_irq.c
+++ b/drivers/gpu/drm/nouveau/nouveau_irq.c
@@ -52,6 +52,7 @@ nouveau_irq_preinstall(struct drm_device *dev)
if (dev_priv-card_type = NV_50) {
INIT_WORK(dev_priv-irq_work, nv50_display_irq_handler_bh);
INIT_WORK(dev_priv-hpd_work, nv50_display_irq_hotplug_bh);
+ spin_lock_init(dev_priv-hpd_state.lock);
INIT_LIST_HEAD(dev_priv-vbl_waiting);
}
}
diff --git a/drivers/gpu/drm/nouveau/nv50_display.c
b/drivers/gpu/drm/nouveau/nv50_display.c
index 83a7d27..0df08e3 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -1014,7 +1014,12 @@ nv50_display_irq_hotplug_bh(struct work_struct *work)
uint32_t unplug_mask, plug_mask, change_mask;
uint32_t hpd0, hpd1 = 0;
- hpd0 = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL) nv_rd32(dev,
NV50_PCONNECTOR_HOTPLUG_INTR);
+ spin_lock_irq(dev_priv-hpd_state.lock);
+ hpd0 = dev_priv-hpd_state.hpd0_bits;
+ dev_priv-hpd_state.hpd0_bits = 0;
+ spin_unlock_irq(dev_priv-hpd_state.lock);
+
+ hpd0 = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_INTR);
if (dev_priv-chipset = 0x90)
hpd1 = nv_rd32(dev, 0xe074) nv_rd32(dev, 0xe070);
@@ -1058,7 +1063,6 @@ nv50_display_irq_hotplug_bh(struct work_struct *work)
helper-dpms(connector-encoder, DRM_MODE_DPMS_OFF);
}
- nv_wr32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL, nv_rd32(dev,
NV50_PCONNECTOR_HOTPLUG_CTRL));
if (dev_priv-chipset = 0x90)
nv_wr32(dev, 0xe074, nv_rd32(dev, 0xe074));
@@ -1072,8 +1076,13 @@ nv50_display_irq_handler(struct drm_device *dev)
uint32_t delayed = 0;
if (nv_rd32(dev, NV50_PMC_INTR_0) NV50_PMC_INTR_0_HOTPLUG) {
- if (!work_pending(dev_priv-hpd_work))
- queue_work(dev_priv-wq, dev_priv-hpd_work);
+ uint32_t hpd0_bits = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL);
+ nv_wr32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL, hpd0_bits);
+ spin_lock(dev_priv-hpd_state.lock);
+ dev_priv-hpd_state.hpd0_bits |= hpd0_bits;
+ spin_unlock(dev_priv-hpd_state.lock);
+
+ queue_work(dev_priv-wq, dev_priv-hpd_work);
}
while (nv_rd32(dev, NV50_PMC_INTR_0) NV50_PMC_INTR_0_DISPLAY) {
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] nouveau: Acknowledge HPD irq in handler, not bottom half
On Wed, Nov 10, 2010 at 5:10 PM, Ben Skeggs bske...@redhat.com wrote:
On Wed, 2010-11-10 at 16:32 -0500, Andy Lutomirski wrote:
The old code generated an interrupt storm bad enough to completely
take down my system.
This only fixes the bits that are defined nouveau_regs.h. Newer hardware
uses another register that isn't described, and I don't have that hardware
to test.
Thanks for looking at this. I'll take a closer look at the problem
today and see what I can come up with too, that'll work with the newer
hardware too.
It should be as simple as adding an hpd1 field to the hpd_state and
making exactly the same change. (It would be nice to put the register
definitions into nouveau_regs.h as well -- I didn't really want to
muck around with a bunch of magic numbers that I can't test.)
I tried writing 0x to the display IRQ control in the handler
to explicitly acknowledge the IRQ, but either I did it wrong or it had
no effect.
I imagine that this explains the unreproducible crashes I had on F13 as well.
--Andy
Ben.
Signed-off-by: Andy Lutomirski l...@mit.edu
Cc: sta...@kernel.org
---
drivers/gpu/drm/nouveau/nouveau_drv.h | 5 +
drivers/gpu/drm/nouveau/nouveau_irq.c | 1 +
drivers/gpu/drm/nouveau/nv50_display.c | 17 +
3 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
b/drivers/gpu/drm/nouveau/nouveau_drv.h
index b1be617..b6c62cc 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -531,6 +531,11 @@ struct drm_nouveau_private {
struct work_struct irq_work;
struct work_struct hpd_work;
+ struct {
+ spinlock_t lock;
+ uint32_t hpd0_bits;
+ } hpd_state;
+
struct list_head vbl_waiting;
struct {
diff --git a/drivers/gpu/drm/nouveau/nouveau_irq.c
b/drivers/gpu/drm/nouveau/nouveau_irq.c
index 794b0ee..b62a601 100644
--- a/drivers/gpu/drm/nouveau/nouveau_irq.c
+++ b/drivers/gpu/drm/nouveau/nouveau_irq.c
@@ -52,6 +52,7 @@ nouveau_irq_preinstall(struct drm_device *dev)
if (dev_priv-card_type = NV_50) {
INIT_WORK(dev_priv-irq_work, nv50_display_irq_handler_bh);
INIT_WORK(dev_priv-hpd_work, nv50_display_irq_hotplug_bh);
+ spin_lock_init(dev_priv-hpd_state.lock);
INIT_LIST_HEAD(dev_priv-vbl_waiting);
}
}
diff --git a/drivers/gpu/drm/nouveau/nv50_display.c
b/drivers/gpu/drm/nouveau/nv50_display.c
index 83a7d27..0df08e3 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -1014,7 +1014,12 @@ nv50_display_irq_hotplug_bh(struct work_struct *work)
uint32_t unplug_mask, plug_mask, change_mask;
uint32_t hpd0, hpd1 = 0;
- hpd0 = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL) nv_rd32(dev,
NV50_PCONNECTOR_HOTPLUG_INTR);
+ spin_lock_irq(dev_priv-hpd_state.lock);
+ hpd0 = dev_priv-hpd_state.hpd0_bits;
+ dev_priv-hpd_state.hpd0_bits = 0;
+ spin_unlock_irq(dev_priv-hpd_state.lock);
+
+ hpd0 = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_INTR);
if (dev_priv-chipset = 0x90)
hpd1 = nv_rd32(dev, 0xe074) nv_rd32(dev, 0xe070);
@@ -1058,7 +1063,6 @@ nv50_display_irq_hotplug_bh(struct work_struct *work)
helper-dpms(connector-encoder, DRM_MODE_DPMS_OFF);
}
- nv_wr32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL, nv_rd32(dev,
NV50_PCONNECTOR_HOTPLUG_CTRL));
if (dev_priv-chipset = 0x90)
nv_wr32(dev, 0xe074, nv_rd32(dev, 0xe074));
@@ -1072,8 +1076,13 @@ nv50_display_irq_handler(struct drm_device *dev)
uint32_t delayed = 0;
if (nv_rd32(dev, NV50_PMC_INTR_0) NV50_PMC_INTR_0_HOTPLUG) {
- if (!work_pending(dev_priv-hpd_work))
- queue_work(dev_priv-wq, dev_priv-hpd_work);
+ uint32_t hpd0_bits = nv_rd32(dev,
NV50_PCONNECTOR_HOTPLUG_CTRL);
+ nv_wr32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL, hpd0_bits);
+ spin_lock(dev_priv-hpd_state.lock);
+ dev_priv-hpd_state.hpd0_bits |= hpd0_bits;
+ spin_unlock(dev_priv-hpd_state.lock);
+
+ queue_work(dev_priv-wq, dev_priv-hpd_work);
}
while (nv_rd32(dev, NV50_PMC_INTR_0) NV50_PMC_INTR_0_DISPLAY) {
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] nouveau: Acknowledge HPD irq in handler, not bottom half
On Wed, 2010-11-10 at 17:25 -0500, Andrew Lutomirski wrote:
On Wed, Nov 10, 2010 at 5:10 PM, Ben Skeggs bske...@redhat.com wrote:
On Wed, 2010-11-10 at 16:32 -0500, Andy Lutomirski wrote:
The old code generated an interrupt storm bad enough to completely
take down my system.
This only fixes the bits that are defined nouveau_regs.h. Newer hardware
uses another register that isn't described, and I don't have that hardware
to test.
Thanks for looking at this. I'll take a closer look at the problem
today and see what I can come up with too, that'll work with the newer
hardware too.
It should be as simple as adding an hpd1 field to the hpd_state and
making exactly the same change. (It would be nice to put the register
definitions into nouveau_regs.h as well -- I didn't really want to
muck around with a bunch of magic numbers that I can't test.)
Yes, it is. I can confirm the problem on another card, but it doesn't
actually cause any crashes here. If you can rework the patch to support
the newer chips too, that'd be great.
As for magic numbers, the register names for those regs are wrong
anyway. The joy of reverse-engineering the support. It doesn't really
matter if you want to stick to them or go back to magic numbers.
Ben.
I tried writing 0x to the display IRQ control in the handler
to explicitly acknowledge the IRQ, but either I did it wrong or it had
no effect.
I imagine that this explains the unreproducible crashes I had on F13 as well.
--Andy
Ben.
Signed-off-by: Andy Lutomirski l...@mit.edu
Cc: sta...@kernel.org
---
drivers/gpu/drm/nouveau/nouveau_drv.h |5 +
drivers/gpu/drm/nouveau/nouveau_irq.c |1 +
drivers/gpu/drm/nouveau/nv50_display.c | 17 +
3 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
b/drivers/gpu/drm/nouveau/nouveau_drv.h
index b1be617..b6c62cc 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -531,6 +531,11 @@ struct drm_nouveau_private {
struct work_struct irq_work;
struct work_struct hpd_work;
+ struct {
+ spinlock_t lock;
+ uint32_t hpd0_bits;
+ } hpd_state;
+
struct list_head vbl_waiting;
struct {
diff --git a/drivers/gpu/drm/nouveau/nouveau_irq.c
b/drivers/gpu/drm/nouveau/nouveau_irq.c
index 794b0ee..b62a601 100644
--- a/drivers/gpu/drm/nouveau/nouveau_irq.c
+++ b/drivers/gpu/drm/nouveau/nouveau_irq.c
@@ -52,6 +52,7 @@ nouveau_irq_preinstall(struct drm_device *dev)
if (dev_priv-card_type = NV_50) {
INIT_WORK(dev_priv-irq_work, nv50_display_irq_handler_bh);
INIT_WORK(dev_priv-hpd_work, nv50_display_irq_hotplug_bh);
+ spin_lock_init(dev_priv-hpd_state.lock);
INIT_LIST_HEAD(dev_priv-vbl_waiting);
}
}
diff --git a/drivers/gpu/drm/nouveau/nv50_display.c
b/drivers/gpu/drm/nouveau/nv50_display.c
index 83a7d27..0df08e3 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -1014,7 +1014,12 @@ nv50_display_irq_hotplug_bh(struct work_struct
*work)
uint32_t unplug_mask, plug_mask, change_mask;
uint32_t hpd0, hpd1 = 0;
- hpd0 = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL) nv_rd32(dev,
NV50_PCONNECTOR_HOTPLUG_INTR);
+ spin_lock_irq(dev_priv-hpd_state.lock);
+ hpd0 = dev_priv-hpd_state.hpd0_bits;
+ dev_priv-hpd_state.hpd0_bits = 0;
+ spin_unlock_irq(dev_priv-hpd_state.lock);
+
+ hpd0 = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_INTR);
if (dev_priv-chipset = 0x90)
hpd1 = nv_rd32(dev, 0xe074) nv_rd32(dev, 0xe070);
@@ -1058,7 +1063,6 @@ nv50_display_irq_hotplug_bh(struct work_struct *work)
helper-dpms(connector-encoder, DRM_MODE_DPMS_OFF);
}
- nv_wr32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL, nv_rd32(dev,
NV50_PCONNECTOR_HOTPLUG_CTRL));
if (dev_priv-chipset = 0x90)
nv_wr32(dev, 0xe074, nv_rd32(dev, 0xe074));
@@ -1072,8 +1076,13 @@ nv50_display_irq_handler(struct drm_device *dev)
uint32_t delayed = 0;
if (nv_rd32(dev, NV50_PMC_INTR_0) NV50_PMC_INTR_0_HOTPLUG) {
- if (!work_pending(dev_priv-hpd_work))
- queue_work(dev_priv-wq, dev_priv-hpd_work);
+ uint32_t hpd0_bits = nv_rd32(dev,
NV50_PCONNECTOR_HOTPLUG_CTRL);
+ nv_wr32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL, hpd0_bits);
+ spin_lock(dev_priv-hpd_state.lock);
+ dev_priv-hpd_state.hpd0_bits |= hpd0_bits;
+ spin_unlock(dev_priv-hpd_state.lock);
+
+ queue_work(dev_priv-wq, dev_priv-hpd_work);
}
while (nv_rd32(dev, NV50_PMC_INTR_0) NV50_PMC_INTR_0_DISPLAY) {
Re: [PATCH 2/2] nouveau: Acknowledge HPD irq in handler, not bottom half
On Wed, Nov 10, 2010 at 5:35 PM, Ben Skeggs bske...@redhat.com wrote: On Wed, 2010-11-10 at 17:25 -0500, Andrew Lutomirski wrote: On Wed, Nov 10, 2010 at 5:10 PM, Ben Skeggs bske...@redhat.com wrote: On Wed, 2010-11-10 at 16:32 -0500, Andy Lutomirski wrote: The old code generated an interrupt storm bad enough to completely take down my system. This only fixes the bits that are defined nouveau_regs.h. Newer hardware uses another register that isn't described, and I don't have that hardware to test. Thanks for looking at this. I'll take a closer look at the problem today and see what I can come up with too, that'll work with the newer hardware too. It should be as simple as adding an hpd1 field to the hpd_state and making exactly the same change. (It would be nice to put the register definitions into nouveau_regs.h as well -- I didn't really want to muck around with a bunch of magic numbers that I can't test.) Yes, it is. I can confirm the problem on another card, but it doesn't actually cause any crashes here. If you can rework the patch to support the newer chips too, that'd be great. As for magic numbers, the register names for those regs are wrong anyway. The joy of reverse-engineering the support. It doesn't really matter if you want to stick to them or go back to magic numbers. That explains why INTR and CTRL seemed backwards :) I'll leave the magic numbers for the 0xe07? stuff. Also, I accidentally dropped the enabled_bits part -- I'll put that back. Patch to follow after I boot and test it here. --Andy ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] nouveau: Acknowledge HPD irq in handler, not bottom half
On Wed, Nov 10, 2010 at 11:51 PM, Andrew Lutomirski l...@mit.edu wrote: On Wed, Nov 10, 2010 at 5:35 PM, Ben Skeggs bske...@redhat.com wrote: On Wed, 2010-11-10 at 17:25 -0500, Andrew Lutomirski wrote: On Wed, Nov 10, 2010 at 5:10 PM, Ben Skeggs bske...@redhat.com wrote: On Wed, 2010-11-10 at 16:32 -0500, Andy Lutomirski wrote: The old code generated an interrupt storm bad enough to completely take down my system. This only fixes the bits that are defined nouveau_regs.h. Newer hardware uses another register that isn't described, and I don't have that hardware to test. Thanks for looking at this. I'll take a closer look at the problem today and see what I can come up with too, that'll work with the newer hardware too. It should be as simple as adding an hpd1 field to the hpd_state and making exactly the same change. (It would be nice to put the register definitions into nouveau_regs.h as well -- I didn't really want to muck around with a bunch of magic numbers that I can't test.) Yes, it is. I can confirm the problem on another card, but it doesn't actually cause any crashes here. If you can rework the patch to support the newer chips too, that'd be great. As for magic numbers, the register names for those regs are wrong anyway. The joy of reverse-engineering the support. It doesn't really matter if you want to stick to them or go back to magic numbers. That explains why INTR and CTRL seemed backwards :) I'll leave the magic numbers for the 0xe07? stuff. Perhaps remove the bad definitions from the reg file, or rename them to UNKsomething? Also, I accidentally dropped the enabled_bits part -- I'll put that back. Patch to follow after I boot and test it here. --Andy ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Far away from the primal instinct, the song seems to fade away, the river get wider between your thoughts and the things we do and say. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] nouveau: Acknowledge HPD irq in handler, not bottom half
On Wed, 2010-11-10 at 17:51 -0500, Andrew Lutomirski wrote: On Wed, Nov 10, 2010 at 5:35 PM, Ben Skeggs bske...@redhat.com wrote: On Wed, 2010-11-10 at 17:25 -0500, Andrew Lutomirski wrote: On Wed, Nov 10, 2010 at 5:10 PM, Ben Skeggs bske...@redhat.com wrote: On Wed, 2010-11-10 at 16:32 -0500, Andy Lutomirski wrote: The old code generated an interrupt storm bad enough to completely take down my system. This only fixes the bits that are defined nouveau_regs.h. Newer hardware uses another register that isn't described, and I don't have that hardware to test. Thanks for looking at this. I'll take a closer look at the problem today and see what I can come up with too, that'll work with the newer hardware too. It should be as simple as adding an hpd1 field to the hpd_state and making exactly the same change. (It would be nice to put the register definitions into nouveau_regs.h as well -- I didn't really want to muck around with a bunch of magic numbers that I can't test.) Yes, it is. I can confirm the problem on another card, but it doesn't actually cause any crashes here. If you can rework the patch to support the newer chips too, that'd be great. As for magic numbers, the register names for those regs are wrong anyway. The joy of reverse-engineering the support. It doesn't really matter if you want to stick to them or go back to magic numbers. That explains why INTR and CTRL seemed backwards :) I'll leave the magic numbers for the 0xe07? stuff. That sounds good, it'll all get a cleanup at some point and switched to proper (well, our best guess, you'd have to ask NVIDIA about the real ones) names. Ben. Also, I accidentally dropped the enabled_bits part -- I'll put that back. Patch to follow after I boot and test it here. --Andy ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] nouveau: Acknowledge HPD irq in handler, not bottom half
On Wed, Nov 10, 2010 at 5:55 PM, Maarten Maathuis madman2...@gmail.com wrote: On Wed, Nov 10, 2010 at 11:51 PM, Andrew Lutomirski l...@mit.edu wrote: On Wed, Nov 10, 2010 at 5:35 PM, Ben Skeggs bske...@redhat.com wrote: On Wed, 2010-11-10 at 17:25 -0500, Andrew Lutomirski wrote: On Wed, Nov 10, 2010 at 5:10 PM, Ben Skeggs bske...@redhat.com wrote: On Wed, 2010-11-10 at 16:32 -0500, Andy Lutomirski wrote: The old code generated an interrupt storm bad enough to completely take down my system. This only fixes the bits that are defined nouveau_regs.h. Newer hardware uses another register that isn't described, and I don't have that hardware to test. Thanks for looking at this. I'll take a closer look at the problem today and see what I can come up with too, that'll work with the newer hardware too. It should be as simple as adding an hpd1 field to the hpd_state and making exactly the same change. (It would be nice to put the register definitions into nouveau_regs.h as well -- I didn't really want to muck around with a bunch of magic numbers that I can't test.) Yes, it is. I can confirm the problem on another card, but it doesn't actually cause any crashes here. If you can rework the patch to support the newer chips too, that'd be great. As for magic numbers, the register names for those regs are wrong anyway. The joy of reverse-engineering the support. It doesn't really matter if you want to stick to them or go back to magic numbers. That explains why INTR and CTRL seemed backwards :) I'll leave the magic numbers for the 0xe07? stuff. Perhaps remove the bad definitions from the reg file, or rename them to UNKsomething? Well, they're known. One is hotplug detect enable (unless the code is wrong) and the other is hotplug interrupt status. --Andy ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] nouveau: Acknowledge HPD irq in handler, not bottom half
The old code generated an interrupt storm bad enough to completely
take down my system.
Signed-off-by: Andy Lutomirski l...@mit.edu
---
drivers/gpu/drm/nouveau/nouveau_drv.h |6 +
drivers/gpu/drm/nouveau/nouveau_irq.c |1 +
drivers/gpu/drm/nouveau/nv50_display.c | 35 +++
3 files changed, 33 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
b/drivers/gpu/drm/nouveau/nouveau_drv.h
index b1be617..c926d88 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -531,6 +531,12 @@ struct drm_nouveau_private {
struct work_struct irq_work;
struct work_struct hpd_work;
+ struct {
+ spinlock_t lock;
+ uint32_t hpd0_bits;
+ uint32_t hpd1_bits;
+ } hpd_state;
+
struct list_head vbl_waiting;
struct {
diff --git a/drivers/gpu/drm/nouveau/nouveau_irq.c
b/drivers/gpu/drm/nouveau/nouveau_irq.c
index 794b0ee..b62a601 100644
--- a/drivers/gpu/drm/nouveau/nouveau_irq.c
+++ b/drivers/gpu/drm/nouveau/nouveau_irq.c
@@ -52,6 +52,7 @@ nouveau_irq_preinstall(struct drm_device *dev)
if (dev_priv-card_type = NV_50) {
INIT_WORK(dev_priv-irq_work, nv50_display_irq_handler_bh);
INIT_WORK(dev_priv-hpd_work, nv50_display_irq_hotplug_bh);
+ spin_lock_init(dev_priv-hpd_state.lock);
INIT_LIST_HEAD(dev_priv-vbl_waiting);
}
}
diff --git a/drivers/gpu/drm/nouveau/nv50_display.c
b/drivers/gpu/drm/nouveau/nv50_display.c
index 83a7d27..014f69c 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -1012,11 +1012,18 @@ nv50_display_irq_hotplug_bh(struct work_struct *work)
struct drm_connector *connector;
const uint32_t gpio_reg[4] = { 0xe104, 0xe108, 0xe280, 0xe284 };
uint32_t unplug_mask, plug_mask, change_mask;
- uint32_t hpd0, hpd1 = 0;
+ uint32_t hpd0, hpd1;
- hpd0 = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL) nv_rd32(dev,
NV50_PCONNECTOR_HOTPLUG_INTR);
+ spin_lock_irq(dev_priv-hpd_state.lock);
+ hpd0 = dev_priv-hpd_state.hpd0_bits;
+ dev_priv-hpd_state.hpd0_bits = 0;
+ hpd1 = dev_priv-hpd_state.hpd1_bits;
+ dev_priv-hpd_state.hpd1_bits = 0;
+ spin_unlock_irq(dev_priv-hpd_state.lock);
+
+ hpd0 = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_INTR);
if (dev_priv-chipset = 0x90)
- hpd1 = nv_rd32(dev, 0xe074) nv_rd32(dev, 0xe070);
+ hpd1 = nv_rd32(dev, 0xe070);
plug_mask = (hpd0 0x) | (hpd1 16);
unplug_mask = (hpd0 16) | (hpd1 0x);
@@ -1058,10 +1065,6 @@ nv50_display_irq_hotplug_bh(struct work_struct *work)
helper-dpms(connector-encoder, DRM_MODE_DPMS_OFF);
}
- nv_wr32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL, nv_rd32(dev,
NV50_PCONNECTOR_HOTPLUG_CTRL));
- if (dev_priv-chipset = 0x90)
- nv_wr32(dev, 0xe074, nv_rd32(dev, 0xe074));
-
drm_helper_hpd_irq_event(dev);
}
@@ -1072,8 +1075,22 @@ nv50_display_irq_handler(struct drm_device *dev)
uint32_t delayed = 0;
if (nv_rd32(dev, NV50_PMC_INTR_0) NV50_PMC_INTR_0_HOTPLUG) {
- if (!work_pending(dev_priv-hpd_work))
- queue_work(dev_priv-wq, dev_priv-hpd_work);
+ uint32_t hpd0_bits, hpd1_bits = 0;
+
+ hpd0_bits = nv_rd32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL);
+ nv_wr32(dev, NV50_PCONNECTOR_HOTPLUG_CTRL, hpd0_bits);
+
+ if (dev_priv-chipset = 0x90) {
+ hpd1_bits = nv_rd32(dev, 0xe074);
+ nv_wr32(dev, 0xe074, hpd1_bits);
+ }
+
+ spin_lock(dev_priv-hpd_state.lock);
+ dev_priv-hpd_state.hpd0_bits |= hpd0_bits;
+ dev_priv-hpd_state.hpd1_bits |= hpd1_bits;
+ spin_unlock(dev_priv-hpd_state.lock);
+
+ queue_work(dev_priv-wq, dev_priv-hpd_work);
}
while (nv_rd32(dev, NV50_PMC_INTR_0) NV50_PMC_INTR_0_DISPLAY) {
--
1.7.3.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
